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

Fix units from transports appearing at load point. #16938

Merged
merged 2 commits into from Aug 18, 2019

Conversation

@tovl
Copy link
Contributor

commented Aug 11, 2019

Fixes #16901

The MoveIntoWorld activity must not be interruptible and Enter should always be allowed to exit gracefully on its own because that would otherwise cause other glitches. So instead make sure that Enter chooses the current location for exiting and just let that handle the unload movement.

@pchote pchote added this to the Next Release milestone Aug 12, 2019

@tovl tovl force-pushed the tovl:transport-glitch branch 2 times, most recently from 7f62b62 to 2a983fe Aug 12, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

Renamed EnterTransport to RideTransport and removed all calls to MoveIntoWorld from UnloadCargo as well as all Production traits.

MoveIntoWorld is now always called on actor creation instead and is changed to be a proper (noninterruptible) activity. This seems to have solved #16930 as well.

OpenRA.Test/OpenRA.Test.csproj Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Aug 16, 2019

The transport changes look reasonable, but forcing the MoveIntoWorld activity for all actors regresses some of the non-production actor creation cases.

Aug-16-2019 17-59-39

Notice how the paradrop plane does a loop when entering the map, and the dropped actors don't open their parachutes until after they touch down.

This could be fixed by changing DelayInit into a MoveIntoWorld init, and only queue the activity if it is explicitly set.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Fixed the issues with aircraft and paratroopers. Note that these issues where not actually exclusive to non-production cases. The MoveIntoWorld method for aircraft just generally didn't make much sense before (and wasn't used either), so I changed it to cover the actions that an aircraft actually would do when moving into the world.

@pchote
Copy link
Member

left a comment

My gut feeling is still that always forcing the MoveIntoWorld activity is going to cause problems, but (a) I can't think of any specific examples off the top of my head and (b) fixing this by only queueing the activity if the Init is defined is a simple future fix. I don't see much point wasting both our time banging heads over that without a firmer basis - but please address the naming nits below:

OpenRA.Mods.Common/ActorInitializer.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Mobile.cs Outdated Show resolved Hide resolved
OpenRA.Test/OpenRA.Test.csproj Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:transport-glitch branch from 5d19055 to b75c0fd Aug 17, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

Applied fix-ups and updated authors list.

@pchote
Copy link
Member

left a comment

Code changes look reasonable aside from one small typo. I'll try to get to test this later tonight.

OpenRA.Mods.Common/Traits/Air/Aircraft.cs Outdated Show resolved Hide resolved

@tovl tovl force-pushed the tovl:transport-glitch branch from b75c0fd to 966109f Aug 17, 2019

@tovl tovl force-pushed the tovl:transport-glitch branch from 966109f to c5a5e6e Aug 17, 2019

@tovl tovl force-pushed the tovl:transport-glitch branch from c5a5e6e to f293b7d Aug 17, 2019

@tovl tovl force-pushed the tovl:transport-glitch branch from f293b7d to fa99f5e Aug 17, 2019

@tovl tovl force-pushed the tovl:transport-glitch branch from fa99f5e to 85d1386 Aug 17, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

This seems to have broken the shellmap, and possibly other scripts that script units loaded into transports (reported from the latest test build).

Repro case:

  • Set Timestep = <something small> in Session.cs and comment out the forceRender = true line in Game.cs to speed up testing
  • Run the shellmap for several effective game minutes and notice that the Soviet infantry which are unloaded from transports begin to pile up, idle, outside the allied base.
@pchote

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

False alarm: with my sped up test case i've been able to reproduce this on bleed, so can't be a regression here.

@abcdefg30
Copy link
Member

left a comment

Looks good to me otherwise.

@tovl tovl force-pushed the tovl:transport-glitch branch from 85d1386 to ecd8a82 Aug 18, 2019

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Aug 18, 2019

Style nits fixed.

False alarm: with my sped up test case i've been able to reproduce this on bleed, so can't be a regression here.

😥

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

Note: double false alarm, because the same issue also exists in release-20190314 and earlier 😌

@abcdefg30 abcdefg30 merged commit 3fe78a8 into OpenRA:bleed Aug 18, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Aug 18, 2019

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