(WIP) Fuel accuracy fix for #37943 #37960
Conversation
ghost
requested a review
from 
curstwist
added
<Bugfix>
There was a problem hiding this comment.
While I am moderately supportive of "I don't understand this part of the vehicle code at all, so I'm going to rip it out and replace it with something I do understand", I am not convinced that you understand the new code and I am fairly sure I do understand the old code.
A couple of points on your testing procedure:
- You should really need to drive for a minute when you're collecting your fuel consumption data. Just use a tiny amount of fuel (like 0.5L) by filling an empty tank with a plastic bottle's worth of fuel.
- You're going to need to pass the vehicle efficiency tests when you're done, and I'm not convinced that this code will do so.
Really, the best way to handle this would be to log actual fuel consumption in a 10 slot circular buffer and then project the 10 second running average. Doing so would be a little complicated and I remember having difficulty the one time I half-heartedly took to a shot at it.
| } | ||
| int energy_j_per_mL = fuel.fuel_energy() * 1000; | ||
| return -amount_pct * fuel_rate_w / energy_j_per_mL; | ||
| int energy_j_per_L = fuel.fuel_energy() * 1000; |
There was a problem hiding this comment.
fuel.fuel_energy() returns MJ/L
basic dimensional analysis:
1 MJ/L * 1,000,000 J/MJ * 1/1000 L/mL = 1000 J/mL
1 MJ/L * 1,000,000 J/MJ * 1 L/L = 1,000,000 J/L
So mathematically, the old code was correct and the new code is wrong.
| int energy_j_per_mL = fuel.fuel_energy() * 1000; | ||
| return -amount_pct * fuel_rate_w / energy_j_per_mL; | ||
| int energy_j_per_L = fuel.fuel_energy() * 1000; | ||
| return -3600 * ( fuel_rate_w / energy_j_per_L ); |
There was a problem hiding this comment.
What are you even trying to do here? Okay, maybe reporting nominal fuel consumption at a fixed speed isn't a great solution, but vehicles also don't normally run at 100% of their engine capacity.
| // otherwise have been division-by-zero | ||
| debugmsg( "Vehicle unexpectedly has zero acceleration" ); | ||
| } else { | ||
| amount_pct += 600 * vslowdown / accel; |
There was a problem hiding this comment.
this should probably be amount_pct += 3600 * vslowdown / accel; and the comment starting with // HACK: should be removed now that it is no longer true.
| double remainder = fuel_remainder[ ft ]; | ||
| amnt_precise_j -= remainder; | ||
|
|
||
| if( amnt_precise_j > 0.0f ) { | ||
| fuel_remainder[ ft ] = drain_energy( ft, amnt_precise_j ) - amnt_precise_j; | ||
| double actual_drain = drain_energy( ft, amnt_precise_j ); |
There was a problem hiding this comment.
Why are you messing with the functions that handle fuel consumption in a PR that is supposed to be about displaying fuel consumption?
If this function is buggy, open a separate issue and a PR to fix to it, don't do it in this one please.
| // calculate fuel consumption for the lower of safe speed or 70 mph | ||
| // or 0 if the vehicle is idling | ||
| if( is_moving() ) { | ||
| int target_v = std::min( safe_velocity(), 70 * 100 ); |
There was a problem hiding this comment.
Thinking about this, almost no one drives at 70 mph, which is a brisk 17 tiles/turn. Calculating nominal fuel consumption at 40 mph would probably be better.
|
Thanks for the feedback @mlangsdorf! I'm going to close this PR and re-open a simpler one that addresses the off-by-6x issue and changes the calculations to 40mph. |
|
I tried going back and simply fixing the 6x issue and switching to 40mph, and the remaining time estimates were still off by an order of magnitude at least. I'm going to back off of this one and leave it to the experts for now! |
Summary
SUMMARY: Interface "Improve accuracy of remaining fuel time"Purpose of change
Fixes #37943
I'm marking this WIP because it still isn't accurate, but it's way more accurate than before and I'm not sure it will ever be.
The idea of this change is to use the actual last-tick fuel consumption data to estimate remaining fuel time. However, I'm not an expert in physics and the formulas were really mind-bending. I think I have the general idea right but I'm not certain and am PR'ing this to get some expert feedback.
Describe the solution
Instead of the old calculation, which seemed to be quite far off the mark, I'm using the last-tick fuel consumption number to calculate remaining fuel.
Describe alternatives you've considered
I nearly gave up after several hours of reading the code and doing road tests.
Testing
I spawned a very long straight road using overmap specials, and then spawned diesel, gas, and electric vehicles. I siphoned the tanks down to < 0.5L for the gas/diesel, and installed small car batteries in the electrics, and then ran them at various speeds and looked at the remaining time.
At the moment, it looks like it is still slightly under-estimating the time remaining for electric vehicles, and it gets numbers that bounce up and down a little for gas/diesel, but it appears to be way closer than before at least.
Additional context
I took this on for my first real C++ challenge on this project. I may have bitten off more than I can chew.