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

Pause MovePart when Mobile is paused. #17189

Merged
merged 1 commit into from Oct 6, 2019

Conversation

@tovl
Copy link
Contributor

commented Oct 6, 2019

This was an oversight from #16262

Fixes #17043

@pchote

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

As mentioned in #17043, I'm on the fence about whether this is the best approach.

If we restore pausing stopping movement immediately we will be forced to introduce a third state for Mobile (using Subterranean/Jumpjet locomotors) and Aircraft that disables player input but allows the unit to continue moving to land or surface.

IMO "reject player input and move to a safe/valid location before stopping" is a completely reasonable default behaviour for pausing, and cases like the Hijacker that really do want to freeze the unit immediately have the option of enabling a speed modifier.

What do we gain by going this route instead?

Copy link
Contributor

left a comment

Fix works.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

This fixes the regression part and restores the thief to its current (stable) behaviour. The other use cases are not part of stable mods yet, so do not require a quick fix.

I agree that always going to a valid location instead of stopping on the spot is the more sane behaviour, but there are several snags in the current movement logic that prevent that from working properly (especially with regards to turning). I have been busy for a while now with a rigorous rework of the movement logic that will (hopefully) address those issues in a more robust way, but that is way out of scope for next release. In the mean time, I am hesitant to add more workarounds that may make that job more difficult.

I am also not sure if a proper implementation of the hijacker behaviour would absolutely need freezing on the spot either and would be more happy if there where no exceptions to the rule at all (so speed modifiers would also only apply after the current MovePart has ended).

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2019

(so speed modifiers would also only apply after the current MovePart has ended)

Please no. Ares's AttachEffect-based SpeedMultiplier: 0 case freezes units on the spot and that distinction has usecases within the modding scene.

@tovl

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2019

So what does that do to units coming up behind?

@pchote pchote added this to the Next Release milestone Oct 6, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

In that case I agree that it makes sense to restore the previous behavior and defer discussion/other changes until your rework is ready.

@pchote
pchote approved these changes Oct 6, 2019
@pchote pchote added the PR: Needs +2 label Oct 6, 2019
@pchote pchote merged commit eed00de into OpenRA:bleed Oct 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.