Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign up[CR] Floating point velocity and new properties. #23602
Conversation
Isaacssv552
added some commits
Apr 24, 2018
This comment has been minimized.
This comment has been minimized.
|
Why is the builder failing? It compiles fine locally. |
This comment has been minimized.
This comment has been minimized.
|
The std::abs() thing in bionics.cpp is pretty straightforward, is guess it's a debug vs release build thing. Overall looks like a good chunk of changes to split out if the main pr. |
This comment has been minimized.
This comment has been minimized.
|
@kevingranade The odd thing is that I did both release and debug builds locally without errors. |
AMurkin
reviewed
Apr 25, 2018
| /** Get wheel radius or return 0 if part is not wheel */ | ||
| float vehicle_part::wheel_radius() const | ||
| { | ||
| return base.is_wheel() ? base.type->wheel->diameter : 0; |
This comment has been minimized.
This comment has been minimized.
Isaacssv552
added some commits
Apr 25, 2018
ZhilkinSerg
added
<Enhancement / Feature>
[JSON]
Vehicles
[C++]
labels
Apr 25, 2018
BevapDin
reviewed
Apr 25, 2018
| @@ -417,7 +417,7 @@ bool map::vehact( vehicle &veh ) | |||
|
|
|||
| // "Anti-ideal" vehicle slows down up to 10 times faster than ideal one | |||
| const float k_slowdown = 20.0f / ( 2.0f + 9 * ( veh.k_dynamics() * veh.k_mass() ) ); | |||
| const int slowdown = veh.drag() + (int)ceil( k_slowdown * base_slowdown ); | |||
| const float slowdown = veh.drag() + (int)ceil( k_slowdown * base_slowdown ); | |||
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
Now that the result is stored as floating point value, the cast to int and the call to ceil (probably inserted for the conversion) may not be required any more?
This comment has been minimized.
This comment has been minimized.
BevapDin
reviewed
Apr 25, 2018
| @@ -2881,6 +2881,11 @@ units::mass vehicle::total_mass() const | |||
| return mass_cache; | |||
| } | |||
|
|
|||
| float vehicle::nowheel_mass() const | |||
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
This should return something like units::quantity<float, units::mass_in_gram_tag>, same for wheel_mass.
You may also convert all the velocity values to something like quantity<float, velocity_in_meter_per_second_tag> (or similar, matching the actually used unit). Currently one can not easily see the units involved with all those values and one can easily use those values in the wrong context.
BevapDin
reviewed
Apr 25, 2018
|
|
||
| /** | ||
| * Calculates the mass of the wheels of the vehicle. | ||
| * @param boat If true, calculates the area under "wheels" that allow swimming. |
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
It calculates the area when boat is true and the mass when it's false?
This comment has been minimized.
This comment has been minimized.
BevapDin
reviewed
Apr 25, 2018
| ///\EFFECT_STR caps vehicle weight for muscle engines | ||
| const units::mass move_mass = std::max( g->u.str_cur * 25_gram, 150_gram ) * 1000; | ||
| const units::mass move_mass = std::max( g->u.str_cur * 25_gram, 150_gram ) * 1000.0f; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
Btw. the multiplication with a floating point instead of an integer is pointless. It creates a temporary quantity<float, mass_in_gram_tag> instance that is assigned to a units::mass, which is int-based, so the floating point value is converted back to an integer.
This comment has been minimized.
This comment has been minimized.
Isaacssv552
Apr 26, 2018
Author
Contributor
Didn't realize units::mass had to be an integer. Thanks!
BevapDin
reviewed
Apr 25, 2018
| { | ||
| if( ( engine_on && has_engine_type_not( fuel_type_muscle, true ) ) || skidding ) { | ||
| return safe_velocity( fueled ) * k_mass() / ( 1 + strain () ) / 10; | ||
| return safe_velocity( fueled ) * k_mass() / ( 1.0f + strain () ) / 10.0f; |
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
These changes are similar pointless: strain returns float, 1 + strain() is a float as well, the multiplication will be done using floating point variables anyway.
This comment has been minimized.
This comment has been minimized.
Isaacssv552
Apr 26, 2018
Author
Contributor
I prefer to change all numbers to the expected return type when possible. I find it improves code readability. When the types are mixed it forces the reader to spend time considering type/operator precedence to calculate the result. e.g.
int cls::fn() // Returns <10
...
float foo = fn / 10; // Always 0.
float foo = fn / 10.0f; // Functions as expected.
BevapDin
reviewed
Apr 25, 2018
| float total_mass = 0; | ||
| const auto &wheel_indices = boat ? floating : wheelcache; | ||
| for( auto &wheel_index : wheel_indices ) { | ||
| // Convert grams to kilograms. |
This comment has been minimized.
This comment has been minimized.
BevapDin
reviewed
Apr 25, 2018
| return base.is_wheel() ? base.weight().value() : 0; | ||
| } | ||
|
|
||
| /** Get wheel friction or return 0 if part is not wheel */ |
This comment has been minimized.
This comment has been minimized.
BevapDin
Apr 25, 2018
Contributor
Why is the documentation (the exact same text) in two files? You only need it in one (typically the header). Why add all the unnecessary redundancy?
This comment has been minimized.
This comment has been minimized.
Isaacssv552
Apr 26, 2018
Author
Contributor
I think it was that way in the original and I assumed it was expected. These utility functions are all directly based on existing functions.
Coolthulhu
reviewed
Apr 29, 2018
| * Calculates the average radius of the wheels of the vehicle. | ||
| * @param boat If true, calculates the area under "wheels" that allow swimming. | ||
| */ | ||
| float wheel_radius( bool boat ) const; |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
reviewed
Apr 29, 2018
| @@ -342,13 +342,19 @@ struct islot_wheel | |||
|
|
|||
| /** width of wheel (inches) */ | |||
| int width = 0; | |||
|
|
|||
| /** coefficient of friction (percent) */ | |||
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Apr 29, 2018
Contributor
The value it is replacing is diameter*width, so contact area. But here you use it as if it was unitless.
Coolthulhu
reviewed
Apr 29, 2018
| }; | ||
|
|
||
| struct islot_fuel | ||
| { | ||
| public: | ||
| /** Energy of the fuel (kilojoules per charge) */ | ||
| float energy = 0.0f; | ||
|
|
||
| /** Air to fuel ratio. */ | ||
| float ratio = 1.0f; |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Apr 29, 2018
Contributor
Don't add values you don't intend to use in the same PR, unless it's a lot of code and you need to split things up.
Keep it for a PR that actually uses the values.
codemime
reviewed
May 5, 2018
| * Calculates the mass of the wheels of the vehicle. | ||
| * @param boat If true, calculates the area under "wheels" that allow swimming. | ||
| */ | ||
| float wheel_mass( bool boat ) const; |
This comment has been minimized.
This comment has been minimized.
codemime
May 5, 2018
Member
wheel_mass( true ) doesn't seem to be utilized. Boolean flags are best avoided wherever possible, especially in case of a single argument. They make reading harder (one needs to see the declaration to learn the meaning) and clearly state that a function does more than one thing.
codemime
reviewed
May 5, 2018
| @@ -701,8 +701,8 @@ void map::move_vehicle( vehicle &veh, const tripoint &dp, const tileray &facing | |||
| // Find collisions | |||
| // Velocity of car before collision | |||
| // Split into vertical and horizontal movement | |||
| const int &coll_velocity = vertical ? veh.vertical_velocity : veh.velocity; | |||
| const int velocity_before = coll_velocity; | |||
| const float &coll_velocity = vertical ? veh.vertical_velocity : veh.velocity; | |||
This comment has been minimized.
This comment has been minimized.
codemime
reviewed
May 5, 2018
| @@ -4307,13 +4327,13 @@ bool vehicle::collision( std::vector<veh_collision> &colls, | |||
| } | |||
|
|
|||
| const bool vertical = bash_floor || dp.z != 0; | |||
| const int &coll_velocity = vertical ? vertical_velocity : velocity; | |||
| const float &coll_velocity = vertical ? vertical_velocity : velocity; | |||
This comment has been minimized.
This comment has been minimized.
codemime
reviewed
May 5, 2018
| @@ -6351,19 +6374,38 @@ bool vehicle_part::fault_set( const fault_id &f ) | |||
| return true; | |||
| } | |||
|
|
|||
| int vehicle_part::wheel_area() const | |||
| /** Get wheel area or return 0 if part is not wheel */ | |||
| float vehicle_part::wheel_area() const | |||
This comment has been minimized.
This comment has been minimized.
codemime
May 5, 2018
Member
wheel_area() is used in one place only. I'd drop it to make the interface a wee bit narrower than it is now. Also, the condition below is useless. Both wheel_diameter() and wheel_width() would return 0 for non-wheels.
codemime
reviewed
May 5, 2018
| } | ||
|
|
||
| /** Get wheel radius or return 0 if part is not wheel */ | ||
| float vehicle_part::wheel_radius() const |
This comment has been minimized.
This comment has been minimized.
codemime
May 5, 2018
Member
Why keep both wheel_diameter() and wheel_radius() around simultaneously? I'd leave only one.
This comment has been minimized.
This comment has been minimized.
|
Seems stalled. |
Isaacssv552 commentedApr 24, 2018
•
edited
This is split from #23560. It simply switches velocity from int to float and adds some (unused)JSON properties and utility functions.
Contents: