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 Chronoshifted units not returning to their origin #15509

Merged
merged 5 commits into from Aug 15, 2018

Conversation

Projects
None yet
4 participants
@pchote
Member

pchote commented Aug 12, 2018

Fixes #8670
Fixes #13059
Fixes #15497

There are 3 cases where the Teleport activity can fail:

  1. The unit is a deployed MAD Tank (via the IPreventsTeleport interface)
  2. There are no free cells within a caller-defined radius of the target. Note that for the chrono-return case the range is 50 cells, so this should never occur in practice.
  3. Described by the source code comment, and also described with screenshots to help in #8670:
    // The Move activity is not immediately cancelled, which, combined
    // with Activity.Cancel discarding NextActivity without checking the
    // IsInterruptable flag, means that a well timed order can cancel the
    // Teleport activity queued below - an exploit / cheat of the return mechanic.
    
    This is the one that causes all our problems.

This PR removes the first case by replacing the IPreventsTeleport interface with conditions on Chronoshiftable. The second case is handled by killing the actor in the rare event that it may happen (so there should be no meaningful impact on gameplay behaviour). The third case is worked around using a dirty but IMO well explained and justified hack in the last commit.

Note that the Chronoshift code lives in Mods.Cnc, and is generally poor quality - the whole thing needs a rewrite. Implementing this hack gives us a pragmatic medium-term fix until the activity and chronoshift code can be rewritten properly.

@pchote pchote added this to the Next release milestone Aug 12, 2018

@matjaeck

MAD tank:

<<+pchote>> there are two changes there: the chronoshift bar now disappears on deploy instead of ticking down and then doing nothing, and it shows the "cannot move" cursor instead of the normal move cursor and then doing nothing when ordered

#8670 / #15497:
Spamming commands no longer blocks the chrono-return.

#15368 seems to be be fixed too, at least I couldn't reproduce it after trying it multiple times (maybe my timing is just not as good as @abcdefg30's).

LGTM

@GraionDilach

👍

@pchote pchote merged commit 20dbf76 into OpenRA:bleed Aug 15, 2018

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
Member

pchote commented Aug 15, 2018

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