Vehicle boat/traction update #17687

Coolthulhu · Jul 14, 2016

Mostly refactoring for future land vehicle/boat split.
Wheel configuration, traction etc. functions got a bool boat argument. If false, it checks the wheels. If true, the boat parts.

Other code changes:

Removed hardcoded division by 9 of wheel area. Adjusted k_mass so that it doesn't change from current values.
Removed some hardcoded boat checks
Changed the "nothing grabbed" condition from grab_point == tripoint_zero to grab_type == OBJECT_NONE. This allows grabbing stuff below self

Gameplay changes:

Updated vehicle push/pull. Had to, because vehicle push/pull is related to all of: traction, wheel configuration, pivoting. All of those were changed here, so vehicle grab could easily regress otherwise. On the positive side, the changes make dragging bicycles a much less painful experience for the user - I didn't manage to lose my grabbed bike by walking around.
Added a "K traction" display to vehicle interaction menu. It displays the multiplier on acceleration (1.0 or less) due to bad traction, calculated for dirt (the most common terrain for offroading).
Added a warning message when acceleration of a vehicle is 0. Can happen when biking on a heavy vehicle, for example.

mugling · Jul 17, 2016

Needs rebase

Coolthulhu · Jul 18, 2016

Rebased

mugling · Jul 21, 2016

On the positive side, the changes make dragging bicycles a much less painful experience for the user

This is a significant improvement

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/item.cpp

@@ -3531,6 +3531,15 @@ std::set<fault_id> item::faults_potential() const
    return res;
 }

+int item::wheel_area() const
+{
+    if( !is_wheel() ) {


overly verbose. item.cpp is already obese.

return is_wheel() ? type->wheel->diameter * type->wheel->width : 0

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/game.cpp

+    const tripoint player_prev = u.pos();
+    u.setpos( tripoint_zero );
+    if( grabbed_vehicle->collision( colls, dp_veh, true ) ) {
+        add_msg( _("The %s collides with %s."),


mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/veh_interact.cpp

+    bool bal_boat = veh->balanced_wheel_config( true );
+    float steer = veh->steering_effectiveness();
+
+    std::string wheel_status;


Could this not be refactored to not depend upon so many stack variables. Possibly via if( is_boat ) ...

I could extract it into a function

Could work. One of the biggest problems our code base has is lengthy functions - some are thousands of lines long!

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

@@ -716,16 +716,20 @@ class vehicle : public JsonSerializer, public JsonDeserializer
    // 1.0 means mass won't slow vehicle at all, 0 - it won't move
    float k_mass () const;

+    // Coefficient of traction
+    // Result depends on wheel traction, wheel area and vehicle's total mass


Best as doxygen comments. Should also specify @return outlining possible range of return values

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

@@ -701,8 +701,8 @@ class vehicle : public JsonSerializer, public JsonDeserializer
    // Loop through engines and generate noise and smoke for each one
    void noise_and_smoke( double load, double time = 6.0 );

-    // Calculate area covered by wheels and, optionally count number of wheels
-    float wheels_area (int *cnt = 0) const;
+    // Calculate area covered by wheels


convert to doxygen comment

mugling · Jul 21, 2016

Vehicle movement functions need some further documentation starting with the k_* family. The work you're doing on this is excellent but it's very hard to follow without such documentation.

Can you provide a skeleton outline on the github wiki of how vehicles actually move as this would probably accelerate review of vehicle PR's and possibly encourage other developers to assist. It doesn't need to be all encompassing but having something there also encourages future contributions to the documentation.

Coolthulhu · Jul 21, 2016

Cleaned up

Not sure how to document the k_*. Their exact values aren't all that relevant, except when we're talking about balance. The way they're used is generally just multiplication.

mugling · Jul 21, 2016

Not sure how to document the k_*

Can you provide an overview/introduction as to how the whole system works?

Coolthulhu · Jul 21, 2016

Can you provide an overview/introduction as to how the whole system works?

Maximum speed is multiplied by k_dynamics and k_mass.
Safe speed is multiplied by k_dynamics and k_mass ^ 2.
Acceleration is multiplied by k_dynamics, k_mass ^2 and k_traction.
k_dynamics = k_friction * k_aerodynamics

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/veh_interact.cpp

@@ -1445,6 +1445,55 @@ void veh_interact::display_veh ()
    wrefresh (w_disp);
 }

+std::string wheel_state_description( const vehicle &veh )


mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

-    float k_dynamics () const;
+    /**
+     * Combined coefficient of aerodynamic and wheel friction resistance of vehicle.
+     * @returns A value from 0.0 to 1.0 (exclusive) describing the dynamics coefficient.


The range is helpful but could be expressed in the more standard notation [0.0-1.0] for inclusive bounds or [0.0-1.0) for exclusive bounds

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

@@ -164,6 +164,9 @@ struct vehicle_part : public JsonSerializer, public JsonDeserializer
    /** Try to set fault returning false if specified fault cannot occur with this item */
    bool fault_set( const fault_id &f );

+    /** Get wheel diameter times wheel width (inches^2) or return 0 if part is not wheel */


UTF-8 symbols are supported in source code and practically any font is going to include superscript 2. Readability is king.

I disagree about UTF where it isn't needed. ^2 is readable enough.

Not essential at all for this PR but out of curiosity why?

Easier to write, search for, display in exotic fonts (say, dedicated roguelike ones).
It's more convenient, like space-less paths even though everything supports spaces in paths.

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

+
+    /**
+     * Air friction coefficient of the vehicle.
+     * @returns A value from 0.0 to 1.0 (exclusive) describing the aerodynamics coefficient.


How is this derived and what are the effects?

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

+    /**
+     * Wheel friction coefficient of the vehicle.
+     * @returns A value from 0.0 to 1.0 (exclusive) describing the friction coefficient.
+     */


This only really explains the range. A short description as to what affects the value and how it affects other properties would be useful.

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

-    // 1.0 means mass won't slow vehicle at all, 0 - it won't move
+    /**
+     * Mass coefficient of the vehicle.
+     * Depends on wheel area.


What are the effects of this? These values are expressed to the UI so at some point external documentation would be a good idea. For now an expanded synopsis would suffice.

Should it really be described in k_* documentation? It sounds like documentation equivalent of breaking encapsulation.

The previous comment (presuming it wasn't removed as misleading) was actually more helpful explaining what the value actually meant.

The vehicle k_* API is basically a set of undocumented magic values masquerading as functions. I know it didn't begin here, but it is being extended here and it is an enormous obstacle to both review and collaboration and undoubtedly frustrating for all.

Encapsulation isn't an argument if these values are exposed as part of the public API and especially in the cases where they are presented directly to the user. If the k_* family truly are implementation details they need to be encapsulated within vehicle and an alternative set of values provided as part of the public API.

If the k_* family truly are implementation details they need to be encapsulated within vehicle and an alternative set of values provided as part of the public API.

Those exist: acceleration, safe_speed etc.

The specific results of k_* functions are implementation details, the functions themselves aren't. They have to be partially magical because they're imprecise approximations of real life data, tuned to return balanced values rather than to represent reality.

Not sure how do you see the structure of those. It's impossible to expose all the details of functions like k_traction (and even if it was possible, it would be a giant block of text that shouldn't be in the documentation anyway). while for k_mass and k_friction it would be just a block of implementation details.

As for documenting where are those used: this doesn't happen pretty much anywhere else. Check out item.h: there are nearly no comments like "it is used in x". Most are just "it returns x", "it changes the item so that it is x", "it returns the x property of the item".

The specific results of k_* functions are implementation details

At least k_aerodynamics, k_friction and k_mass are exposed directly to the user via veh_interact.cpp. That's the polar opposite of an implementation detail. At an absolute minimum those need to be sufficiently defined so that players can actually make decisions based upon those values.

As for documenting where are those used: this doesn't happen pretty much anywhere else

The vehicle class is in places so poorly documented it's almost impossible to work out what's going on. The options are either to document individual functions or provide a synopsis of how all the parts fit together.

At an absolute minimum those need to be sufficiently defined so that players can actually make decisions based upon those values.

Documentation of dispersion and recoil fields doesn't describe how does dispersion and recoil work. Neither do functions that calculate those for loaded weapons.
item::get_layer doesn't describe how layers work in the slightest.
etc.

Documenting how and where are the functions used is fragile and results in "documentation rot".
Unless you mean something like "Affects speed and acceleration" (without specifics), because that's unlikely to change soon.

Documentation of dispersion and recoil fields doesn't describe how does dispersion and recoil work

How dispersion and recoil work is a frequent player question and should really have some documentation written to explain the system. An answer of "it's an implementation detail" would not be reasonable.

Documenting how and where are the functions used is fragile and results in "documentation rot". Unless you mean something like "Affects speed and acceleration" (without specifics), because that's unlikely to change soon.

Define k_aerodynamics, k_friction and k_mass as these are exposed directly to the user

Document the valid range of returns for each k_* function and what the min/max means

Provide an overall synopsis of how vehicles move and what affects that

The latter can be either part of the vehicle class documentation or added to the GH wiki.

Define k_aerodynamics, k_friction and k_mass as these are exposed directly to the user

Those are defined: value from 0 to 1 that describes the mass/friction/etc. coefficient. That's their definition.
Describing how they're used should be done where they are used, not defined.

How dispersion and recoil work is a frequent player question and should really have some documentation written to explain the system.

But not at the item field, no matter how (if at all) it is exposed.
k_mass doesn't mean anything on its own, it only gets a meaning when used to calculate the relevant values. Same for recoil: it only contributes to overall dispersion value, describing the details of accuracy at recoil field would be a giant mistake, just like describing the exact mechanics of how does k_mass contribute to acceleration, speed and speed loss when coasting in documentation of k_mass.

At least it's apparent that increasing dispersion and recoil is a bad thing. The comparison UI even highlights that. I have no idea if increased k_aerodynamics is a good or bad thing?

The preamble for this PR provides a better explanation than your commits:

Added a "K traction" display to vehicle interaction menu. It displays the multiplier on acceleration (1.0 or less) due to bad traction, calculated for dirt (the most common terrain for offroading)

Why did you delete some of the function documentation?

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

    float k_mass () const;

+    /**
+     * Traction coefficient of the vehicle.
+     * Depends on wheel area and surface beneath vehicle's wheels..


And presumably affects skidding and efficiency?

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.cpp

-    float fr0 = 1000.0;
-    float kf = ( fr0 / (fr0 + wheels_area()) );
-    return kf;
+    constexpr float fr0 = 9000.0;


magic value

Was always magic, kept for compatibility.

Any idea what it means?

I assume it's just there for the numbers to look right.
It doesn't really model any real life effect.

Might be best to document is as unknown magical value - at least that makes it clear future removal would be acceptable

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h


-    // Combined coefficient of aerodynamic and wheel friction resistance of vehicle, 0-1.0.
-    // 1.0 means it's ideal form and have no resistance at all. 0 -- it won't move


Does this still hold true because it's more descriptive than the below?

Should I copy+paste "0 means won't move, 1 means ideal" to every k_* description?

No but you could wrap the whole family of k_* functions in a doxygen scope and explain that there

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/vehicle.h

    float k_aerodynamics () const;

-    // Coefficient of mass, 0-1.0.
-    // 1.0 means mass won't slow vehicle at all, 0 - it won't move


Same here. Presuming this remains true the new documentation is worse

Coolthulhu · Jul 21, 2016

How about this?

mugling · Jul 21, 2016

Better but I'd wrap the whole lot in a doxygen scope and provide a short summary.

The function documentation would be better in the style of the preamble one you gave for k_traction

Coolthulhu · Jul 21, 2016

How to scope multiple functions and provide one description for all of them and specific descriptions for details for each function?

mugling · Jul 21, 2016

There is an example starting at item.h:1209

Coolthulhu · Jul 21, 2016

How about this?

mugling · Jul 21, 2016

Perfect.

Now they have definitions the VEHICLE stats dumper would benefit from dumping of those values - can you add them to dump.cpp as it will help in identifying any changes in future PR's.

Apart from that looks good to go

Coolthulhu · Jul 21, 2016

Added the same info that veh_interact.cpp displays to dump.cpp

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/dump.cpp

@@ -147,7 +147,10 @@ void game::dump_stats( const std::string& what, dump_mode mode )

    } else if( what == "VEHICLE" ) {
        header = {
-            "Name", "Weight (empty)", "Weight (fueled)"
+            "Name", "Weight (empty)", "Weight (fueled)",
+            "Max velocity", "Safe velocity", "Acceleration",


What are the units of max velocity, safe velocity and acceleration?

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/dump.cpp

+            r.push_back( to_string( veh_fueled.k_mass() ) );
+            r.push_back( to_string( veh_fueled.k_aerodynamics() ) );
+            r.push_back( to_string( veh_fueled.k_friction() ) );
+            r.push_back( to_string( veh_fueled.k_traction( veh_fueled.wheel_area( false ) / 2.0f ) ) );


Why the division?

Grass has 50% the traction of road right now.

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/dump.cpp

@@ -157,6 +160,13 @@ void game::dump_stats( const std::string& what, dump_mode mode )
            r.push_back( veh_empty.name );
            r.push_back( to_string( veh_empty.total_mass() ) );
            r.push_back( to_string( veh_fueled.total_mass() ) );
+            r.push_back( to_string( veh_fueled.max_velocity() ) );
+            r.push_back( to_string( veh_fueled.safe_velocity() ) );


Might be best formatted to fewer significant figures, possibly also a percentage?

mugling · Jul 21, 2016

mugling reviewed Jul 21, 2016

View changes

mugling · Jul 21, 2016

src/veh_interact.cpp

@@ -1564,6 +1576,8 @@ void veh_interact::display_stats()
        }
    }

+    bool is_boat = !veh->all_parts_with_feature(VPFLAG_FLOATS).empty();
+
    fold_and_print(w_stats, y[7], x[7], w[7], c_ltgray,
                   _("K aerodynamics: <color_ltblue>%3d</color>%%"),


Should be percentages for presentation to the user. A food cart having traction of 16% and a tank 89% is a reasonable UI

The percentages are at the end of the string.

mugling · Jul 21, 2016

k_mass doesn't appear to do much. Is that a regression from this PR or has it always been there?

Coolthulhu · Jul 21, 2016

k_mass doesn't appear to do much.

?

mugling · Jul 21, 2016

Dump the vehicle stats. All vehicles are 99%

Coolthulhu · Jul 21, 2016

I forgot a to include a magic constant I removed from wheels_area in k_mass.

Coolthulhu added some commits Jul 14, 2016

Add traction display to vehicle interaction

87867b6

Don't display 100%

58035ae

Fix vehicle pulling

403d092

Fix boats

7b12751

Fix vehicle push/pull

dcaa29f

Allow braking when accel == 0

9fd355f

Coolthulhu force-pushed the cataclysmbnteam:vehicle-boat-disp branch to 9fd355f Jul 18, 2016

Post-rebase fix

Loading status checks…

605b796

mugling self-assigned this Jul 21, 2016

Coolthulhu added some commits Jul 21, 2016

Cleanup

cc648d7

Wheel area comment

Loading status checks…

d26488c

More cleanup

Loading status checks…

2d3542a

Docs about k_*

Loading status checks…

3b76904

More docs

Loading status checks…

db6bbe8

Coolthulhu added some commits Jul 21, 2016

Add to dump.cpp

ac7c7fa

Tiny fix

Loading status checks…

7249374

Units in dump

Loading status checks…

eabb5a4

Forgotten magic constant

Loading status checks…

f58b43b

mugling merged commit 6f69357 into CleverRaven:master Jul 21, 2016
1 check passed

1 check passed

default
Details

Maddremor referenced this pull request May 2, 2017
Closed
Enable playermade amphibious vehicles #20933

CleverRaven/Cataclysm-DDA

Join GitHub today

Vehicle boat/traction update #17687

Conversation

Coolthulhu commented Jul 14, 2016

This comment has been minimized.

mugling commented Jul 17, 2016

Coolthulhu added some commits Jul 14, 2016

Coolthulhu force-pushed the cataclysmbnteam:vehicle-boat-disp branch to 9fd355f Jul 18, 2016

This comment has been minimized.

Coolthulhu commented Jul 18, 2016

mugling self-assigned this Jul 21, 2016

This comment has been minimized.

mugling commented Jul 21, 2016

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

mugling commented Jul 21, 2016

Coolthulhu added some commits Jul 21, 2016

This comment has been minimized.

Coolthulhu commented Jul 21, 2016

This comment has been minimized.

mugling commented Jul 21, 2016

This comment has been minimized.

Coolthulhu commented Jul 21, 2016

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling Jul 21, 2016 • edited

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mugling Jul 21, 2016 • edited

mugling reviewed Jul 21, 2016 View changes

This comment has been minimized.

mugling Jul 21, 2016 • edited

This comment has been minimized.

Coolthulhu commented Jul 21, 2016

This comment has been minimized.

mugling commented Jul 21, 2016

This comment has been minimized.

Coolthulhu force-pushed the cataclysmbnteam:vehicle-boat-disp branch to `9fd355f` Jul 18, 2016

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling Jul 21, 2016 •

edited

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling Jul 21, 2016 •

edited

mugling reviewed Jul 21, 2016

View changes

mugling Jul 21, 2016 •

edited

mugling commented Jul 21, 2016 •

edited

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling reviewed Jul 21, 2016

View changes

mugling merged commit `6f69357` into CleverRaven:master Jul 21, 2016
1 check passed