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 visual L-turn stutter #15224

Merged
merged 4 commits into from Jul 1, 2018

Conversation

Projects
None yet
4 participants
@reaperrr
Copy link
Contributor

reaperrr commented Jun 9, 2018

Fixes #11885.

More details in the commit descriptions.

@reaperrr reaperrr force-pushed the reaperrr:fix-Lturn-stutter branch from 4524229 to 64b4595 Jun 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 9, 2018

Updated last commit as per IRC discussion.

@reaperrr reaperrr force-pushed the reaperrr:fix-Lturn-stutter branch from 64b4595 to d969644 Jun 9, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 10, 2018

Updated again.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 13, 2018

@pchote @abcdefg30 I know we want to get out a playtest as quickly as possible, but since this actually affects the shipping mods, too (even if barely noticable), I'd like to get this into the playtest unless someone stumbles over some serious regression or conceptual flaw.

@reaperrr reaperrr added this to the Next release milestone Jun 13, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jun 13, 2018

Pinging @GraionDilach for review (or at least confirmation that it fixes #11885 / #11895).

For anyone testing this, best test-case on the shipping mods:
TS, slowest gamespeed, GDI, heavy start units.
Order infantry and mechs around and watch those L-turn-to-somethingelse transitions carefully.
On bleed, you should definitely notice that hiccup, on this PR it should be gone.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Jun 17, 2018

This does fix #11895.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Code also looks alright to me, so 👍

@Smittytron
Copy link
Contributor

Smittytron left a comment

Works


protected override void OnLastRun(Actor self)
{
// If Mobile.IsMoving was set to 'true' OnFirstRun, we want to reset it to 'false' before the next tick.

This comment has been minimized.

@pchote

pchote Jun 22, 2018

Member

This comment isn't correct - if Mobile.IsMoving was true before the activity ran then OnFirstRun won't do anything and it will still be reset false here.

reaperrr added some commits Jun 9, 2018

Fix losing a tick when next Move.ChildActivity is Turn
While the first tick of the MoveFirstHalf child would run at the parent Move tick (see 2nd-to-last line in Move.Tick), this was not the case for Turn.
As a result, this Move tick would get wasted if a Turn was necessary, which at least contibuted to that visible jerk at the end of each L-turn (actors usually don't have the exact facing needed for the next move at the end of an L-turn).
Cache IFacing in Turn activity
Instead of looking it up every tick.
Set Mobile.IsMoving to "true" if queueing a turn that takes only a si…
…ngle tick

At the end of L-turns, actors often end up with an internal facing not 100% matching the direction of the next cell on their path.
As a result, if they haven't reached their destination yet, Move queues a quick Turn as ChildActivity, which previously was not considered as IsMoving.
However, we don't want those mini-turns to interrupt move animations, so we now consider them a move as well. Additionally, to avoid any issues, we make these mini-turns non-interruptible, just like the MovePart activities already are.

@reaperrr reaperrr dismissed stale reviews from Smittytron and GraionDilach via 2685386 Jul 1, 2018

@reaperrr reaperrr force-pushed the reaperrr:fix-Lturn-stutter branch from d969644 to 2685386 Jul 1, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jul 1, 2018

Updated and rebased.

@pchote

pchote approved these changes Jul 1, 2018

Copy link
Member

pchote left a comment

I don't see any obvious fundamental issues here, so lets get it in and hope any regressions shake out sooner rather than later.

@pchote pchote merged commit 4c239d4 into OpenRA:bleed Jul 1, 2018

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:fix-Lturn-stutter branch Aug 5, 2018

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