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

Make sure units move to/stay on the appropriate movement layer after stopping #15949

Merged
merged 2 commits into from Mar 10, 2019

Conversation

@obrakmann
Copy link
Contributor

commented Dec 26, 2018

Supersedes #15919, see the discussion there.

@MustaphaTR

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

Units given stop command on a high-bridge, which below the bridge is also passable by the unit, get a move command to below the bridge, they no longer phase thru them but still (try to) move there. Same doesn't happen for Tunnel because move command is ignored.

@mazarf

This comment has been minimized.

Copy link
Contributor

commented Dec 26, 2018

The high-bridge issue might be related to the Mobile.OnBecomingIdle behavior brought up in #15919.

@obrakmann obrakmann force-pushed the obrakmann:fix-tunnel-weirdness branch from e009626 to b010608 Dec 27, 2018

@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

Yup, it is. Fixed, and added a big-ass comment to explain all this mess.

@mazarf

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

I revisited this and found IMO a simpler way to handle this. Instead of shortening the movement path, this makes the unit stop then immediately gives it an order to leave the tunnel in OnBecomingIdle.

The tunnel exit Location is found in EntersTunnels.CustomLayerChanged. This searches for the closest tunnel entrance and retrieves the exit from that. I'm not sure if this is the best way to find the exit but this was what I came up with.

bleed...mazarf:fix-tunnel-weirdness

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

Just a note that the Move changes here will be made redundant by #16246. We'll need to test the rest of this once that has been merged.

@pchote

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

#16246 has been merged, so this needs a rebase now. I think the ICustomMovementLayer.PreventsStopping is still probably worthwhile for decoupling Move from the specific layer implementations, even if the original bug has been fixed.

@obrakmann obrakmann force-pushed the obrakmann:fix-tunnel-weirdness branch from b010608 to 92ee33c Mar 10, 2019

@obrakmann obrakmann changed the title Fix bugs related to units stopping in tunnels Make sure units move to/stay on the appropriate movement layer after stopping Mar 10, 2019

@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Rebased and updated. This now only fixes the remaining issue where units on high bridges would float to the ground after stopping.

@pchote
Copy link
Member

left a comment

Just one nit, otherwise LGTM

@@ -669,7 +669,10 @@ void IDeathActorInitModifier.ModifyDeathActorInit(Actor self, TypeDictionary ini

void INotifyBecomingIdle.OnBecomingIdle(Actor self)
{
if (TopLeft.Layer == 0)

This comment has been minimized.

Copy link
@pchote

pchote Mar 10, 2019

Member

If we keep the self.Location.Layer == 0 check by itself at the start we can avoid the TraitsImplementing lookup in 99.9% of cases.

@obrakmann obrakmann force-pushed the obrakmann:fix-tunnel-weirdness branch from 92ee33c to 714ec58 Mar 10, 2019

@obrakmann obrakmann added this to the Tiberian Sun Public Alpha milestone Mar 10, 2019

@obrakmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Updated

obrakmann and others added 2 commits Dec 24, 2018
Add a ReturnToGroundLayerOnIdle flag to CustomMovementLayers
Co-authored-by: Mazar Farran <farranmazar@gmail.com>
Co-authored-by: Paul Chote <paul@chote.net>

@obrakmann obrakmann force-pushed the obrakmann:fix-tunnel-weirdness branch from 714ec58 to 349bd8e Mar 10, 2019

@reaperrr reaperrr merged commit 6dd84b2 into OpenRA:bleed Mar 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@obrakmann obrakmann deleted the obrakmann:fix-tunnel-weirdness branch Mar 10, 2019

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