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

Rewrite Enter and remove ResolveFrozenActorOrder. #16150

Merged
merged 19 commits into from Feb 3, 2019

Conversation

Projects
None yet
4 participants
@pchote
Copy link
Member

pchote commented Feb 1, 2019

This PR implements the third and hopefully final part of the target-visibility changes.

The changes required to recreate the old behaviour for #16139 were going to be ugly, so I instead pushed forward and rewrote Enter from scratch. The new activity should support visibility and all our other modern activity expectations. It should also be significantly more readable (not hard, considering the state of the old one).

Fixes #9362.
Fixes #14365.
Fixes #14755.
Fixes #16139.
Closes #16048.
Supersedes #13699.

The Enter activity made up 95% of the work left for #16048, so I included the final four commits to close that here too.

@pchote pchote added this to the Next Release milestone Feb 1, 2019

@pchote pchote force-pushed the pchote:fog-aware-enter branch from 91adefd to f8331c0 Feb 1, 2019

@pchote pchote force-pushed the pchote:fog-aware-enter branch from f8331c0 to 3813f94 Feb 2, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 2, 2019

Updated.

  • Changed Enter method signatures as mentioned above
  • Added a commit to pause movement in MovePart when Mobile is disabled (improves reliability of vehicle capturing).

Note that GitHub has messed up the commit ordering in the web interface as usual.

@matjaeck
Copy link
Contributor

matjaeck left a comment

Can't find any issues with the changes in this PR. #14755 and #16139 are fixed, can't comment on #14365. I tested units that can enter things in RA quite extensively and in-game behavior looks good to me.

@pchote pchote force-pushed the pchote:fog-aware-enter branch from 5b17e3d to 1dea584 Feb 2, 2019

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 2, 2019

Added a new commit to fix #9362, which appears to have a bigger impact now that the Enter activities care about frozen actors (or maybe just being noticed more with people actively testing capturing).

Testcase:

  • Add -SpawnActorsOnSell: to POWR
  • Start a MP game with two players, cheats enabled
  • Player 1 builds a power plant somewhere outside of his normal vision range
  • Player 2 builds a barracks outside his normal vision range, near but outside vision range of the power plant
  • Player 2 builds an engineer and captures the power plant
  • Player 2 sells the power plant

Before the fix: Player 1 sees a power plant frozen actor that has the faction color of Player 2, but a tooltip showing themselves. Player 2 sees a power plant frozen actor that appears to be owned by Player 1.

After the fix: Player 1 sees a power plant frozen actor that appears to be owned by Player 2. Player 2 does not see a frozen actor.

@matjaeck
Copy link
Contributor

matjaeck left a comment

1dea584 fixes #9362. 👍 for in-game behavior.

@pchote pchote added the PR: Needs +2 label Feb 3, 2019

@obrakmann
Copy link
Contributor

obrakmann left a comment

looks ok, and seems to work fine ingame. I tried to test as many edge cases as I could

@obrakmann obrakmann merged commit 2b6ebcd into OpenRA:bleed Feb 3, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

obrakmann commented Feb 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment