Skip to content

/Cataclysm-DDA

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

[WIP] Amphibious vehicles #21723

Closed
wants to merge 4 commits into from

Conversation

Reviewers

@Coolthulhu Coolthulhu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@atlaua @BevapDin @kevingranade @Coolthulhu @BorkBorkGoesTheCode @Leland
@atlaua
Copy link
Contributor

commented Aug 26, 2017

As the title says, I'm working on making amphibious vehicles possible. There's no code for that yet, though; so far I've only done a minor cleanup of the boat code that I'd love to get some feedback on.
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 27, 2017

I've implemented support for amphibious vehicles now; it seems to work fine, but some more testing needs to be done, since this is my first excursion into the depths of the vehicle code.

Also, there's one comfort feature I haven't implemented yet: Currently, all vehicles, including boats, get "is_floating=false" during initialization, which means boats have to be manually set to 'sea' mode by the user after loading a save.

I've tried to recognize boats automatically by adding a g->m.vehicle_wheel_traction( *this ) < 0 check to vehicle:refresh(), but this invariably causes a segfault during save loading that I haven't quite figured out yet. It appears to occur in the 'const auto &tr = ter( pp ).obj();' line in map::vehicle_wheel_traction()`.
@BevapDin

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 27, 2017

I've tried to recognize boats automatically by adding a g->m.vehicle_wheel_traction( *this ) < 0 check [...] but this invariably causes a segfault during save loading

Most likely because the map has not yet been loaded.
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 27, 2017

@BevapDin

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 27, 2017

Vehicles should dynamically (when needed) detect whether they are floating, instead of relying on a member variable. Instead of having a is_floating variable, add a is_floating() function.

atlaua added some commits Aug 26, 2017

@atlaua
refresh() a vehicle whenever one of its parts is broken 
Also, don't ignore the UNMOUNT_ON_DAMAGE flag on tanks. This wasn't a
significant optimization, and avoiding a second is_broken() check makes
the code more readable IMHO.

Currently, this call isn't neccessary since refresh() doesn't care about
the brokenness of parts, but I'm going to change that. Repairing a
broken part already refreshes the vehicle, since it internally removes
the old part and then installs a new one.
fdb22a9
@atlaua
Only add non-broken floating vehicle parts to the 'floating' cache 
The boat code is currently quite inconsistent in that it arbitrarily
considers either all floating parts or only non-broken floating parts in
different parts of the code.

For wheels, this distincion makes some sense (cf. the comment I added to
vehicle::refresh(), but for floating parts is really doesn't. To fix
this, I only include non-broken parts in the 'floating' cache and then
use this cache everywhere instead of resorting to
all_parts_with_feature() calls.
1b95138
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 28, 2017

@kevingranade

This comment has been minimized.

Sign in to view
Copy link
Member

commented Aug 28, 2017

@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 28, 2017

@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 28, 2017

@Coolthulhu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Aug 28, 2017

An 'is_floating()' function would have to loop over all wheels and boat boards, and by my count, it would be called at least three times for every single vehicle movement.

Requiring an UI operation to switch land/float mode is unacceptable. It clearly shows that the implementation must be wrong or incomplete. Player must not be required to manually select the mode.

You will have to set is_floating on the move, in a similar way that falling is set. Like this:

  • When the vehicle is about to move, check if it could move as a land vehicle
  • If it can move as land vehicle, set is_floating = false, then move it as a land vehicle
  • If it would sink as a land vehicle, check if it is also a boat
    • If it is not a boat, sink it
    • If it is a boat, set is_floating = true and move it as a boat

However if is_floating is needed earlier, it may require a more significant redesign.

For purposes of UI and the like, it is tolerable (not good, though) if it displays the last state. It may be worth saving this state to savegame json, to avoid potential weirdness on load.
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 28, 2017 •
edited

@atlaua
Allow vehicles to be amphibious 
To minimize code changes, I introduced a new boolean 'is_floating'
attribute in the vehicle class. Previously, the code treated all
vehicles with floating (that is, VPFLAG_FLOATS) parts as boats; so
change checks for the existence of floating parts to is_floating
conditionals.

is_floating is automatically set to the right mode when an amphibious
vehicle is driven from land into water or vice versa. However, vehicles
can also moved across a shoreline by ramming or dragging. Setting the
is_floating mode automatically in these cases proved too complex for my
taste, so I instead added a user-accessible switch to the vehicle
control menu to be used in these cases.
b1bcbea
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Aug 28, 2017 •
edited

I bit the bullet and made is_floating a function. I would've preferred to cache the floating mode in a variable, but toggling the mode correctly in all cases was getting a bit messy. Everything seems to work fine now, but I need to test and review it more thoroughly.
@atlaua
Use the CoM as pivot point for boats 
Currently, the pivot point of boats is calculated just if it was a land
vehicle, which means the CoM will only be used if no wheels are
installed. This doesn't make a whole lot of sense.

Using the CoM isn't ideal either, but given the current reality
disconnet of the boat code, I don't think we can do much better than
this.
Loading status checks…
d2860b9
@Coolthulhu

Coolthulhu reviewed Sep 11, 2017
View changes

src/vehicle.cpp
@@ -5102,7 +5103,9 @@ void vehicle::refresh()
if( vpi.has_flag( "CAMERA" ) ) {
camera_epower += vpi.epower;
}
if( vpi.has_flag( VPFLAG_FLOATS ) ) {
// The floating cache shouldn't include broken parts;

This comment has been minimized.

Sign in to view
Copy link
@Coolthulhu

Coolthulhu Sep 11, 2017

Contributor

refresh is called only on part addition/removal and similar. floating must necessarily contain both broken and unbroken parts.
@Coolthulhu

Coolthulhu reviewed Sep 11, 2017
View changes

src/vehicle.cpp
remove_part( p );
}

refresh();

This comment has been minimized.

Sign in to view
Copy link
@Coolthulhu

Coolthulhu Sep 11, 2017

Contributor

Forcing refresh here to keep that weird thing with floating not having broken parts sounds really wrong.
Instead make a can_float or something like that, which checks if there is a non-broken floating part or something like that.
@Coolthulhu

Coolthulhu reviewed Sep 11, 2017
View changes

src/vehicle.cpp

// Otherwise, the vehicle is amphibious. We consider it
// to be floating if >2/3 of the wheels are submerged
if ( g->m.vehicle_wheel_traction( *this ) < 0 ) {

This comment has been minimized.

Sign in to view
Copy link
@Coolthulhu

Coolthulhu Sep 11, 2017

Contributor

Negative wheel traction is a hack to sink vehicles. It isn't totally wrong to use it here, but it's not right either.
@Coolthulhu

Coolthulhu reviewed Sep 11, 2017
View changes

src/vehicle.cpp
@@ -6466,3 +6475,24 @@ void vehicle::calc_mass_center( bool use_precalc ) const
mass_center_no_precalc_dirty = false;
}
}

bool vehicle::is_floating() const

This comment has been minimized.

Sign in to view
Copy link
@Coolthulhu

Coolthulhu Sep 11, 2017

Contributor

If you're worried about this being called too often and becoming slow, back it up with a cached variable, just like mass is cached.
@BorkBorkGoesTheCode

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Jan 15, 2018

Is this dead @atlaua ?
@atlaua

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Jan 23, 2018

@BorkBorkGoesTheCode

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 2, 2018

@atlaua what about now?
@Leland

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 2, 2018

Closing for the time being

@Leland Leland closed this Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.