Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vehicle propulsion overhaul #19275

Merged
merged 62 commits into from Nov 19, 2016
Merged

Vehicle propulsion overhaul #19275

merged 62 commits into from Nov 19, 2016

Conversation

mugling
Copy link
Contributor

@mugling mugling commented Nov 13, 2016

Overview

Most of the vehicle code is heavily interdependent so apologies for PR size. I'd appreciate interim review and will probably progress from here to writing acceptance tests.

Current problems

  • Fuel types and effects are hard-coded
  • Fuel usage is too low and isn't affected by either different engines or speed
  • Safe velocity is far too high and maximum velocity is unobtainable
  • Serious balance problems (bigger is better leading to V12 wheelchair)
  • Minimal documentation with plenty of magic values and arbitrary units
  • Two divergent systems for controlling vehicles with lots of menu-spam

Rework goals

  • Semi-realistic physics implementation using SI metric units internally (m/s etc)
  • Usable display units broadly comparable to real-life examples (horsepower, watts etc)
  • Allow definition of new engines and fuel types in JSON
  • Meaningful difference between engines and fuel types
  • Single intuitive system for controlling vehicles

Implementation

Gearing

gears

Combustion engines are only operable within a narrow range of speeds with gears necessary to extend the usable range. This PR implements gearing for engines using the following syntax:

    "type": "ENGINE",
    "gears": [ 3.166, 1.882, 1.296, 0.972, 0.738 ],
    "idle": 800,
    "optimum": 2000,
    "redline": 4000

All gearboxes in the cataclysm are automatics and will automatically shift to try and keep as close to the optimum as possible. Once you run out of gears the only option for more speed is to increase rpm which is both fuel inefficient and results in damage above redline.

Different engines are now useful for differing purposes - for example a heavy vehicle will benefit from a powerful engine with low gearing. Engine damage is now much more sensible - you can put a V12 on a wheelchair and achieve some impressive speed but the engine is rapidly damaged due to high rpm.

If you omit gears then a continuously variable transmission is assumed with the efficiency remaining stable independent of road speed. Electrical engines find their niche here (see also below).

JSON stats

diesel

Much more can now be specified in JSON, in particular:

  • ENGINE:power is specified and displayed in JSON (internally converted to watts)
  • ENGINE:efficiency controls maximum efficiency at optimum rpm
  • ENGINE:fuel allows implementation of engines using novel fuel types
  • AMMO:energy specifies energy density of fuel (in kJ per charge)
  • Starts migrating fuel properties to json flags, for example diesel engines gain COLD_START

Display units are intended to be meaningful and support comparison between items

Physics model

  • Is based around kinetic energy (Ek = ½ mv²)
  • All internal units are being migrated to SI metric (m/s, watts etc)
  • vehicle mass is now far more significant
  • 1 tile/turn = 10mph

Before:

Name Max velocity (mph) Safe velocity (mph)
Bicycle 0 0
Car 196 65
Humvee 515 141
Motorcycle 78 38
Sports Car 956 321

After:

Name Max velocity (mph) Safe velocity (mph)
Bicycle 17 17
Car 57 57
Humvee 35 35
Motorcycle 180 97
Sports Car 152 136

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

Tangent: Generic vehicle tanks plus JSON engines = steam engine

@Coolthulhu
Copy link
Contributor

I really dislike the idea of gears. It sounds like pure complexity with no depth to back it up.
Continuous transmission sounds way better.

r.push_back( to_string( veh_fueled.safe_velocity() / 100 ) );
r.push_back( to_string( veh_fueled.acceleration() / 100 ) );
r.push_back( to_string( int( max_vel * 2.237 ) ) );
r.push_back( to_string( int( safe_vel * 2.237 ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why magic multiplier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converts m/s to mph. Could do with naming (along with the hp to watts factor)

}
}
return "th";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untranslateable
It would be better if it returned the whole phrase:

  • "1st"
  • "2nd"
    etc.

Up to maximum expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't think of that, will need to look into it

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

I really dislike the idea of gears. It sounds like pure complexity with no depth to back it up.

The gears are changed automatically for the player.

Continuous transmission sounds way better.

This is what we have already. It leads to V12 wheelchairs.

* 1:3.7 is a fairly typical differential gearing
* 0.15 is a magic constant based upon ~15" tyres
*/
static constexpr float wheel_diff = 3.7 * 0.15;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that certainly sounds like the kind of detail we really, really don't need to be reflecting in the game.
It's pure magic - being based on real life constants doesn't help one bit here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gear values are purposefully never displayed to the player. Naming constants and basing them on something near-reality is much better than the current system of arbitrary undocumented units.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be much better if the constant here was 1 and you pushed the value to json load and then possibly to wheel entries themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work but for now all wheels should be the same whilst we get the rest of the implementation worked out. Huge wheels with good traction but poor top speed could be a good later addition.

// if current velocity greater than new configuration safe speed
// drop down cruise velocity.
int safe_vel = safe_velocity();
int safe_vel = safe_velocity( *eng ) * 2.237 * 100;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you're using that multiplication quite a bit. May be worth pushing up to safe/max_velocity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current velocity is still in arbitrary units so those conversions are needed. I'm going to drop them once velocity is in m/s however that will be a separate PR to avoid this one growing too large

///\EFFECT_STR increases power output of manual engines
if( part_flag( index, "MUSCLE_LEGS" ) ) {
// average man (STR 10) = 1/3 hp
pwr = g->u.str_cur * 0.033 * 745.7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real life constants alert again
Those tend to make balancing harder, resulting in wildly unrealistic effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are a good starting point and I'd prefer to document how I chose them. As I said the next steps is to write acceptance tests. For example what should the top speed of the bicycle be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be much better to push those to json.
Real life constants in the code are hard to adjusts and are a big code smell. Same real life constants in the jsons are way more manageable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, perhaps those parts should become an ENGINE with the appropriate stats?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then where to specify those values?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use engine power for the base, though scaling needs another number.

It can't be a pure linear conversion. 10 strength isn't 10 times as strong as 1 strength, it's more like 3-4 times as strong as 0 strength, if that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions for:

  • Bicycle speed
  • Wheelchair speed
  • Scaling algorithm


} else if( part_flag( index, VPFLAG_ENGINE ) || part_flag( index, VPFLAG_ALTERNATOR ) ) {
// convert JSON hp into SI metric units (watts)
pwr = vp.info().power * 745.7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you convert it on load?

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

Without gearing the only rational choice is V12 engine and we are back to wheelchairs traveling at mach 3. Anything that attempts to differentiate the engines is defacto gearing (pun intended) so I'd prefer for a semi-realistic system as it's a lot easier to rationalise about.

As to real-world units they can be chucked once we start working on acceptance tests. They are good placeholder values however for now.

How much of the gears implementation we display is up for debate. Currently the gear ratios are hidden (and should remain so) but the current gear and rpm are displayed. The latter is important as it is one the limiting factors for safe velocity.

@Coolthulhu
Copy link
Contributor

The gears are changed automatically for the player.

I mean conceptual complexity. Anyone trying to understand it will need to dig through gear switching code.

This is what we have already. It leads to V12 wheelchairs.

Cap it from both sides.

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

I mean conceptual complexity. Anyone trying to understand it will need to dig through gear switching code.

It's actually really simple - literally just vehicle::gear() and vehicle::rpm()

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

I mean conceptual complexity

Also everything is heavily documented compared to before including the units and there expected ranges. It's a significant nett improvement so a further rewrite isn't an option.

@Coolthulhu
Copy link
Contributor

It's a significant nett improvement so a further rewrite isn't an option.

I disagree, it's a lot of extra complexity of the kind that most players won't care about but will still need to deal with.
I can easily see it being more bad than good.

It would be acceptable if all engines had the same set of gears (say, 1-5), but at the moment this looks like a giant set of false choices masked by implementation details and a load of complexity.

Consider a player who asks a very common question: "which engine is the best for my car?". Without understanding a load of needless details, this question can't be answered with the new, complex system.

What is gained here from that complexity that could not be replicated with a more understandable system?

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

We already have k_traction, k_friction and lots more k_foo which are directly dispalyed to the player, much less intuitive and indeed entirely arbitrary. How specifically are rpm and gear not understandable compared to that?

Consider a player who asks a very common question: "which engine is the best for my car?"

Adding some helpful text in description would help. Eventually we need to support simulating installation of parts to see how properties change although this is more helpful for explaining how k_aerodynamics works as opposed to gear and rpm

What is gained here from that complexity that could not be replicated with a more understandable system?

I'd say this is pretty understandable. You accelerate until you run out of gears and then redline the engine. I can't think of anything conceptually simpler. Sure we could have k_overspeed = 37 derived from power and mass but that's a lot more obtuse.

@mugling mugling closed this Nov 13, 2016
@mugling mugling reopened this Nov 13, 2016
@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

Github really should prompt before close and commit - I obviously meant comment

@hmstanley
Copy link

I love this idea. Since it makes some sense to add this level of detail to vehicles. Right now, it really makes no sense and you just drive, but in my last game I had a 30 ton beast with four engines and I regularly only used one of the four. That doesn't make sense to me. Anyway, I like this addition.

@mugling
Copy link
Contributor Author

mugling commented Nov 13, 2016

To be clear I'm not bashing your original work on the k_* values just making the point that gears and rpm are relatively intuitive to the player. Any system that limits or extends the usable range of an engine, which we do need to have any meaningful engine variety, is gearing by another name.

@Coolthulhu
Copy link
Contributor

We already have k_traction, k_friction and lots more k_foo which are directly dispalyed to the player, much less intuitive and indeed entirely arbitrary. How specifically are rpm and gear not understandable compared to that?

k_whatever are hard to replace. How else would you want to represent heavy vehicle being heavy?
One of them might be scrapeable, which would be a good idea, though.

rpm and gear are just extra complexity that doesn't need to be. Like stuck weapon penalty or like volume-based dispersion multiplier at close range.

It could be replaced with gear range - the only real difference I see here would be avoiding the "stepping" between ratios.

@mugling
Copy link
Contributor Author

mugling commented Dec 7, 2016

@mugling it's even worse then. So, car with top speed 60mph can only travel 3 times faster than walking pedestrian? Isn't that ridiculous?

The only number that matters is tiles per turn. Adjusting how that is rescaled to mph doesn't make anything 'faster' or 'slower'.

@Coolthulhu is entirely correct - there is a finite limit on usable vehicle speeds at which the player can meaningfully control a vehicle. Also there are few advantages to traveling above the speed of the fastest monster.

The implementation is designed with semi-realistic physics but using real-world constants isn't practical for all of the above reasons.

@Aivean
Copy link
Contributor

Aivean commented Dec 7, 2016

@Coolthulhu

I would prefer the third option: realistic max speed which will be still restrained by other things, such as landscape (winding roads, obstacles) and player's reaction/driving skill.

Like in real life: my car can go up to 220mph, but it doesn't mean that I will ever drive that fast.

Regarding the other issues, I think you should agree, that the right approach in designing anything complex is to be consistent. Distance is a fundamental quality of our world, it's something that everyone encounters every day in his/her life. I think it's wise to leave the fundamental constants as close as possible to real world and take shortcuts somewhere else if necessary.

I can imagine fantasy world where sight is limited (due to fog or generally bad vision), you can't hit anything further than 100m away using firearms (due to the winds or bad weapon quality), cars can go 30mph at most... But it's really hard to imagine the world where space is literally bended depending on what you are doing.

@Aivean
Copy link
Contributor

Aivean commented Dec 7, 2016

@mugling

The only number that matters is tiles per turn. Adjusting how that is rescaled to mph doesn't make anything 'faster' or 'slower'.

My point is that it's not possible to "rescale" speed. You have the exact mapping of turns to seconds and you really should have consistent mapping of tiles to meters. Hence the real question is whether it's really necessary to make car speed unrealistically low.

I'd say no. In real life on the narrow residential streets you usually drive at 15-25 mph, which means ~1 map tile per turn, if you assume player tile as 2m.

On parking lots, when there are obstacles around, or when there are speed bumps on the road, you drive very carefully, like less than 5mph (less than 7 player tiles per turn).

I believe it makes sense to allow player to accelerate to any speed he or she wants: 60mph is ~3 map tiles per turn. It may be not a good idea for the player to do that, considering the visibility on the road and road conditions after Cataclysm, but it should be up to the player to decide.

@Coolthulhu
Copy link
Contributor

Regarding the other issues, I think you should agree, that the right approach in designing anything complex is to be consistent.

Consistency is a good thing.
But game design and balance take precedence over realism.
Consistent design and consistent balance are much more important than real world units.

It stays consistent if we assume that houses are stadium-sized and characters are superhuman runners. That is the consistency we're looking for here, not one that tries to translate walking speed (unstated speed) into pedestrian walking speed.
Your calculations involving pedestrian walking speed are actually pretty unrealistic, as they imply that average healthy person can't run faster than 6 km/h.

But it's really hard to imagine the world where space is literally bended depending on what you are doing.

Then don't imagine the space between or around things.
It's like an RTS, where command center is 100 times bigger than the worker that builds it, not 100000 times bigger.

The tile system is inherently non-realistic. We have 4 ton hulks and rats take single tile each. Whole tile.
Trying to cram realism into tile system is just a bad idea.

@Aivean
Copy link
Contributor

Aivean commented Dec 7, 2016

@Coolthulhu

Consistent design and consistent balance are much more important than real world units.

I agree. The only question is where to draw the line?

In my first comment I assumed tile equal to 1.5-2m based on dimensions of common things (not the running speed, which is debatable). When I see double bed in game (2x2 tile) I relate it to 3x3 or even 4x4 meters bed, but not to 50x50 meters bed. Wide vehicles are 5 tiles wide — 7-10 meters or 125 meters?
Two-tiled doors — 3-4 meters or 50 meters? Two-tiled tables?

Having this mapping of tile size to meters means that you have player walking speed at 0.75 mph which is not realistic either, but can be explained (walking carefully, carrying a lot) and in my opinion is better than to have stadium-sized houses and beds.

Anyway, my original question was about top vehicle speed being unrealistic. It appears that car speed is lowered by design. Ok. Maybe someone will mod it then.

@Zireael07
Copy link
Contributor

In my first comment I assumed tile equal to 1.5-2m based on dimensions of common things (not the running speed, which is debatable)

This is roughly correct. However vehicle speed operates on different assumptions as @mugling pointed out. You can't marry the two

@CoroNaut CoroNaut mentioned this pull request Dec 15, 2016
keyspace added a commit to keyspace/Cataclysm-DDA that referenced this pull request Dec 18, 2016
Couldn't find explanation for this in CleverRaven#19275 (vehicle propulsion overhaul).

The small change to `ms_to_display()` touches many places, such
as perceived velocity step while controlling a vehicle, examining
vehicle's optimal/safe/top speeds, etc.

Fixes CleverRaven#19754 (which is already closed due to tangent/off-topic noise).
kevingranade pushed a commit that referenced this pull request Jan 2, 2017
)

Couldn't find explanation for this in #19275 (vehicle propulsion overhaul).

The small change to `ms_to_display()` touches many places, such
as perceived velocity step while controlling a vehicle, examining
vehicle's optimal/safe/top speeds, etc.

Fixes #19754 (which is already closed due to tangent/off-topic noise).
@keyspace
Copy link
Contributor

Note: this PR has been reverted.

@Zireael07
Copy link
Contributor

@keyspace: where's the reverting PR?

@kevingranade
Copy link
Member

kevingranade commented Jan 10, 2017 via email

@Coolthulhu
Copy link
Contributor

Partly my fault, I trusted the fixes to follow up soon, as in my post high above:

OK, I'm merging this, but a revert may be necessary if the fallout is greater than expected. Or possibly even if it's just everything I expected.

In most cases "merge now, fix as next PR" tends to work, I didn't expect this to be such an exception.
@mugling had a good "score" so far and all the slip ups so far were semi-minor UI issues, most of which were fixed within a week.

@CoroNaut
Copy link

CoroNaut commented Jan 12, 2017

Wow. Can I ask what the reasoning behind reverting this and everything else was? As far as I could tell, the only problem was that you were going a little slower than before. I really can't believe that there was this much anger pent up. It isn't right.

@Zireael07
Copy link
Contributor

@CoroNaut: In addition to going slower, multiple engines didn't work, as well as quite a number of other things.

@mugling
Copy link
Contributor Author

mugling commented Jan 12, 2017

In most cases "merge now, fix as next PR" tends to work, I didn't expect this to be such an exception.
@mugling had a good "score" so far and all the slip ups so far were semi-minor UI issues, most of which were fixed within a week.

Still the case - there were a series of PR's that were closing the issues but the last was left open for over a month (#19629) without merge, rejection or indeed any comment at all. The preamble for the linked PR outlines what it fixes and what would be addressed next. With neither merge nor further review there isn't any obvious way to progress.

Other developers are free to draw whatever inference they wish from this and recent events without anybody else further fueling the issue (pun intended).

@kevingranade
Copy link
Member

kevingranade commented Jan 13, 2017 via email

@mugling
Copy link
Contributor Author

mugling commented Jan 13, 2017

As far as I could tell, you had no intention of addressing the speed, fuel efficiency, or multiple engine breakage, which were the critical issues.

The public record shows a lengthy series of PR's concerning the vehicle code along with continuing closure of associated issues until #19629 stalls uncontended.

@CoroNaut
Copy link

It didn't break the game. Given time, it would have been a great addition to the game and would've added some depth into the engines. It was a large undertaking from the start and would require much longer to work out the problems. If it was a simple 100 lines changed, there would not be a lot to look at, but when you have multiple changes everywhere overlapping each other, it becomes time consuming. I do agree that it was merged a bit early with the bugs that it had, but the experimental players have shown their support in reporting the bugs they've found with it. This is the entire reason we put these things into an experimental version, to test, bugfix, verify and test again. It's a process that was stopped halfway with this addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet