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

[WIP] Addressing vehicle's diagonal walls #26739

Closed
wants to merge 35 commits into from

Conversation

4 participants
@y2s82
Copy link
Contributor

commented Nov 17, 2018

Summary

SUMMARY: Bugfixes "Adds sentinel_part aimed to address the diagonal vehicle having broken walls"

Purpose of change

Fixes #5684 - This is the implementation of the idea proposed in the issue by @kevingranade. It will spawn sentinel_part, a derived class of vehicle_part that will act as an extension of the original, although it will exist in its own during its lifespan.

Describe the solution

This introduces sentinel_part, a derived class of vehicle_part, that will be spawned when .turn_dir of a vehicle reaches certain threshold and removed when outside of the threshold. It should be triggered while .turn() is executed. As a derived class, it will make a shallow copy of the original it is copying, retaining the original state and methods. Upon destruction, it will shallow copy over its own changed values to the original. It will not manage resources as the sentinel_part exists only as an extension to the original vehicle_part it copied from.

Describe alternatives you've considered

The solution from the issue seemed like a good compromise to address the issue. I had also observed discussion of introducing different flags to trigger impassible state, but I am unsure how to trigger the behavior.

EDIT:
I have decided to scrap the entire sentinel_part idea. Instead, taking @mlangsdorf's suggestion to abandon the initial approach and go head-on against the problem, I am tinkering with the entire vehicle_part. Chiefly, it will have a pointer to another vehicle_part that will take on the role of the sentinel_part and all of the function will not have to check if self is a sentinel or has an active sentinel associated.

I guess we'll see how that goes.

@mlangsdorf
Copy link
Contributor

left a comment

Should be astyled to match the rest of the code.

Looking forward to seeing you do more with this.

@@ -375,8 +375,10 @@ struct vehicle_part {
* this part.
*/
item_group::ItemList pieces_for_broken_part() const;

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Nov 17, 2018

Contributor

Please don't add unnecessary whitespace.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

Awesome, glad to see someone pick this up.
How are you planning on handling persistence of these?
The main two approaches I can see are either refreshing the existence of these on some regular basis to they get automatically generated based on vehicle state, meaning they can be treated as ephemeral, or treating them as a vehicle part that gets added and removed, but is otherwise persistent, in which case you'll need to write them out to the save file, including their relationship to their parent part.

@y2s82

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2018

Awesome, glad to see someone pick this up.

Thanks :) I hope I can do the justice

How are you planning on handling persistence of these?

I was thinking adding and removing, triggered by vehicle object's .turn() method.
My current idea is rather crude. I make a copy of the parts the sentinel_part is mimicking. The sentinel_part also maintains a reference to the actual vehicle_part. It will start off with the identical stat via a shallow copy. When the object gets deleted, it will run a destructor that copies its possibly-changed information back onto the original part. As far as the life of the sentinel_part, however, it was going to be on its own. I am not sure what would happen if some item gets stored into the compartment. I am hoping it will exist in both and gets removed from both when the user takes it out.

The main two approaches I can see are either refreshing the existence of these on some regular basis to they get automatically generated based on vehicle state, meaning they can be treated as ephemeral, or treating them as a vehicle part that gets added and removed, but is otherwise persistent, in which case you'll need to write them out to the save file, including their relationship to their parent part.

I also thought of an alternative of making them exists always but somehow toggle visibility and/or pass-ability, but I wasn't sure how to go about it.

How does ephemeral work in this game, by the way? Could you point me to an example, perhaps?

I am also not sure how the save file works. Currently, the sentinel_part has a pointer to the base vehicle_part object. Is that sufficient, or does save game portion require some type of transformation?

Also, what may be the most efficient way to find the edges? I was thinking of using get_avail_parts() and then somehow track all of the x and y values to figure it out, but I was worried how it might impact performance whenever the vehicle would turn.

@mlangsdorf

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

Don't use get_avail_parts(). That would ignore boards on carried vehicles as well as broken boards. You want get_any_parts(). Or you would if you were going to spontaneously generate this stuff on each turn, which you shouldn't.

You'll want to add a new cache vector that will get updated in vehicle::refresh() during the get_all_parts() loop, before the is_unavailable() check. Something like:

if( vpi.has_flag( VPFLAG_OBSTACLE) && vpi.has_flag( VPFLAG_OPAQUE ) ) {
    for( size_t adj = 0; adj < 2; adj++ ) {
        if( parts_at_relative( vp.mount() + cardinal_d[ adj ], false ).empty() ) {
            sidewalls.push_back( p );
        }
    }
}

you might need to loop through cardinal_d from 2 to 4, I always get the vehicle mount directions confused.

The vehicle code caches stuff like side wall locations, and refreshes those caches in refresh(). So use that idiom.

Then in turn(), you can loop through the sidewall cache and add the sentinel parts.

@kevingranade

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

Now that i think of it, the pointer will not work, because parts can shift in the vehicle part vector.

Your best bet IMO is to have the sentinel part store the vehicle-relative coordinates of its adjacent part, that at least is stable.

@y2s82

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Thank you both for great ideas. I will look into those functions and consider them into the design. Sometime Monday or Tuesday, I would like to go over and make a thorough check on what I can do and post another message regarding my progress.

@y2s82

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Now that i think of it, the pointer will not work, because parts can shift in the vehicle part vector.

I would like to know more about what you mean. When you say shift, you mean a different pointer may occupy that vector element location, or do you mean the object of the pointer suddenly store different information?

Your best bet IMO is to have the sentinel part store the vehicle-relative coordinates of its adjacent part, that at least is stable.

Should the sentinel_part then constantly recheck if it is making a copy of the correct part each time drawn?

@y2s82

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2018

Don't use get_avail_parts(). That would ignore boards on carried vehicles as well as broken boards. You want get_any_parts(). Or you would if you were going to spontaneously generate this stuff on each turn, which you shouldn't.

I would like to generate it on the first instance it enters the threshold, and removed the moment it leaves the threshold. Given @kevingranade 's comments, however, it seems I may need a mechanism to remove and reattach a new part when things shift about.

You'll want to add a new cache vector that will get updated in vehicle::refresh() during the get_all_parts() loop, before the is_unavailable() check. Something like:

if( vpi.has_flag( VPFLAG_OBSTACLE) && vpi.has_flag( VPFLAG_OPAQUE ) ) {
    for( size_t adj = 0; adj < 2; adj++ ) {
        if( parts_at_relative( vp.mount() + cardinal_d[ adj ], false ).empty() ) {
            sidewalls.push_back( p );
        }
    }
}

Thank you. I will look into everything about this code. :)

you might need to loop through cardinal_d from 2 to 4, I always get the vehicle mount directions confused.

So does the cardinal_d keeps it consistent even if the vehicle is facing different direction than east?

The vehicle code caches stuff like side wall locations, and refreshes those caches in refresh(). So use that idiom.

Okay. I will look at refresh() code and get more understanding. Thanks for the pointer.

Then in turn(), you can loop through the sidewall cache and add the sentinel parts.

@mlangsdorf

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2018

A vehicle has a vector of vehicle_parts, with each part independently keeping track of its x,y position on the vehicle, independent of vehicle facing. Parts can be added or removed at any time, so a vehicle part's position in the vector may decrease and you can't rely on the idea that parts[9] is military composite armor plating at [-1,2] past a call to part_removal_cleanup(). But part_removal_cleanup() calls refresh(), so you can rebuild your cache then.

cardinal_d is simply an array of points { [ 0, 1 ], [ 0, -1], [ 1, 0 ], [ -1, 0 ] }. If you have a part at mount p1 and you want to know what vehicle_parts are cardinally adjacent to it, you can use parts_at_relative( mount, cardinal_d[ i ] ) to loop through them.

My point about caching the sidewalls is you don't want to be generating the vector of sidewall parts every time a vehicle turns. You want to rebuild it during refresh(), with all the other caches, and then immediately iterate through it if necessary. ie, if we have this vehicle:

/=\
|#|
+=+

with (simplified) parts vector [ { "se_frame", [ 0, 0 ], { "se_halfboard", [ 0, 0 ] }, { "center_frame", [ -1, -1 ] }, { "s_frame", [ -1, 0 ] }, { "n_frame", [ -1, -2 ] }, { "nw_frame", [ -2, -2 ] }, { "ne_frame", [ 0, -2 ] }, { "w_frame", [ -2, -1 ] }, { "e_frame", [ 0, -1 ] }, { "sw_frame", [ -2, 0 ] }, { "sw_halfboard", [ -2, 0 ] }, { "e_board", [ 0, -1 ] }, { "s_board", [ -1, 0 ] }, { "w_board", [ -2, -1 ] } ] then during refresh, you create a sidewall_cache with indices [ 1, 10, 11, 13 ]. Then you would check if the vehicle wasn't facing a cardinal direction and iterate through the sidewall cache and add your sentinel parts.

Thinking about this more, you have to do the sentinel parts in vehicle::refresh(), because someone could be inside a completely stationary vehicle (with no wheels, engine, etc) that was skewed. It should still have the sentinel parts but to a reasonable degree, stationary vehicles don't go through the vehicle movement code.

sentinel_part::~sentinel_part()
{
mount = original->mount;
*original = ( vehicle_part ) * this;

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Nov 28, 2018

Contributor

if that's supposed to be a cast, please use C++ style static cast.

* Sentinel Parts that is used to block diagnal openings created when vehicles are at an angle.
* The sentinel_part serves as a special type of vehicle_part with a specific purpose of being used only to block passage 'through' the walls of the vehicle.
* It is designed to be dynamically created on a vehicle_part*, with the constructor copying over the value from the original, and removed when nolonger needed with the constructor who also takes care of the transfer of datasets.
*/

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Nov 28, 2018

Contributor

I don't think this implementation is sufficient. The sentinel part is supposed to be the original, not a copy of the original. As you have it here, the sentinel and the original part take damage separately, effectively doubling the durability of a vehicle's walls when skewed. That's not right.
ie.

S/=\S
 S|p1S    G
  S+=+S

If the NPC gunner at G shoots the player at P, there should only be a single vehicle windshield at 1 to block the shot, even though there's a sentinel part also in the way to clip movement.

This comment has been minimized.

Copy link
@y2s82

y2s82 Nov 29, 2018

Author Contributor

I was aware of each existing separately. I wasn't sure how to implement it while keeping the code sane to maintain. I was hoping that, the sentinel_part copying its information back to vehicle_part when the destructor is ran, would cover the situation when it gets removed or destroyed.

Alternative ideas was to

  • have a public function that will copy over the information back to the original more frequently (perhaps triggered on each refresh() in a loop?),
  • or override all vehicle_part functions of current and future (I hope not),
  • or create not a derived but a separate object that is more or less a clone of the vehicle_part except it would have instances that are references to the original vehicle_part.

Above were becoming more and more complex and difficult to maintain. So I had decided to update when the destructor is ran for now. It certainly is subpar.

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Nov 30, 2018

Contributor

The problem is that there are two vehicle parts - maybe because I don't follow your implementation - and when one of them loses HP or is destroyed, the other should lose HP or be destroyed immediately, not when the vehicle straightens out.

This comment has been minimized.

Copy link
@y2s82

y2s82 Dec 1, 2018

Author Contributor

I agree. My idea is a bit of a work-around. It creates a copy, and then copies over its final stats back to the original at destruction. I made an assumption also that the destructor would be called if the part gets destroyed, which then would copy itself back to original and trigger the original's destruction as well.

Certainly, however, It won't update the original until the destructor is ran, and I would imagine there will be some annoying loopholes the users may exploit like stopping the car in a tilted position and trying to remove the sentinel_part for extra parts, and then regenerate sentinel_part or something.

Of course, I got to work more on it. I think I am understanding it better, in no small parts thanks to you.

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 1, 2018

Contributor

As far as I can tell, you're not adding sentinel parts to the parts vector. So to a large extent, I'm not sure how this is supposed to work - vehicle_parts that aren't in the parts vector aren't in the vehicle for a vast majority of the vehicle code.

I get that you're trying to do this with the minimal amount of impact, but it's going to have a lot of impact and I think it would be better if you addressed it head on. Damaging the sentinel part has to immediately damage the original part, and damaging the original part has to immediately damage the sentinel part. Destroying or removing one of the parts has to immediately destroy or remove the other part. Anything else will lead to all kinds of weird exploits, like double armor by skewing your vehicle or being able to generate an infinite number of certain parts by skewing your vehicle, removing a part, and skewing it again. I would rather not check in a bunch of code that we know has obvious exploits to it.

This comment has been minimized.

Copy link
@y2s82

y2s82 Dec 1, 2018

Author Contributor

I understand.
And no, I had not put any sentinel_parts yet. I wasn't sure how to find the edges. I was able to understand how that works last night. I will do more on adding the sentinel_parts.

Separately, I will also look into making sentinel_parts more responsive.

@@ -250,6 +250,16 @@ void vehicle::turn( int deg )
}
// quick rounding the turn dir to a multiple of 15
turn_dir = 15 * ( ( turn_dir * 2 + 15 ) / 30 );

if( sentinel_present() ) {

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Nov 28, 2018

Contributor

This logic should be moved to vehicle::refresh().

y2s82 added some commits Dec 1, 2018

@mlangsdorf

This comment has been minimized.

Copy link
Contributor

commented Dec 1, 2018

Please fix whatever is causing your editor to randomly retab parts of the code, and review your submissions to make sure you're not submitting a bunch of irrelevant whitespace churn.

y2s82 added some commits Dec 3, 2018

adjusted vehicle_part constructors to account for the new sentinel de…
…sign, and implemented few functions to help identify if the vehicle_part is a sentinel or not and to set and remove sentinels.
added clear() to sentinels,sidewalls. Also put some work into actuall…
…y adding sentinels into the vehicle when the condition triggers.
@mlangsdorf
Copy link
Contributor

left a comment

Looking good.

Thinking about this more, I realized that refresh() isn't necessarily called while a vehicle is turning. So while the code you have is good and necessary, you'll also need to add and remove sentinels during turns. Which is why moving the add/remove logic to a function is a good idea.

@@ -3896,6 +3898,26 @@ void vehicle::refresh()
static_cast<int>( p ), svpv );
relative_parts[pt].insert( vii, p );

if( vpi.has_flag( VPFLAG_OBSTACLE ) && vpi.has_flag( VPFLAG_OPAQUE ) ) {

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 6, 2018

Contributor

For readability, I think it makes sense to encapsulate this entire if statement in a function call.

Also, should be if( vpi.has_flag( VPFLAG_OBSTACLE ) || vpi.has_flag( VPFLAG_OPAQUE ) ) - you want to catch all of windshields, fullboards, halfboards, and curtains or whatever else blocks like of sight without blocking movement.

@@ -3940,6 +3962,42 @@ void vehicle::refresh()
}
}

/*

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 6, 2018

Contributor

This if also should be in a separate function.

bool vehicle::need_sentinel() const
{
int orientation = abs( face.dir() ) % 90;
return orientation > 29 && orientation < 61 && !sentinel_present();

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 6, 2018

Contributor

should be greater than 14 and less than 76, right? You can get skew on long vehicles starting on the first turn increment.

return pt.base.mod_damage( -( pt.base.max_damage() * qty / pt.info().durability ), dt );
} else {
return false;
rtn = pt.base.mod_damage( -( pt.base.max_damage() * qty / pt.info().durability ), dt );

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 6, 2018

Contributor

Much better!

* Check if sentinel is needed on the sidewalls and act accordingly
*/
if( !sentinel_present() ) {
if( need_sentinel() ) {

This comment has been minimized.

Copy link
@mlangsdorf

mlangsdorf Dec 6, 2018

Contributor

Removed parts shouldn't be in sidewalls, so this should probably be safe. You might want to double check that the sidewall part hasn't been marked for removal to be safe.

@Asmageddon

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

What about having an accessibility bitfield, computed based on the unrotated vehicle for vehicles, and kept intact for terrain?

@kevingranade

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

If you have alternate suggestions, please post them in #5684

@kevingranade

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

Seems to be stalled, feel free to reopen if you're going to pick it back up.

Extended Vehicle Tune-Up automation moved this from In progress to Done Feb 25, 2019

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.