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

Add INotifyMoving interface #15675

Merged
merged 4 commits into from Mar 30, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Oct 3, 2018

And refactor some places to use it.

Dependency for #15746.

Fixes #16332.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from 036c180 to 4d3bd02 Oct 28, 2018

@pchote
Copy link
Member

left a comment

Code changes look generally reasonable, but I'd like to try my luck with requesting a scope creep:

OpenRA.Mods.Cnc/Activities/Leap.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Activities/Leap.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

Tested that TD orca and TS tib fiends continue to work without regressions, so nominally 👍 depending on how the discussion above goes.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch 2 times, most recently from c415292 to 26810e5 Nov 20, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

Updated, and confimed that RA dog leaping still works.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2019

Rebased.

@reaperrr reaperrr removed the PR: Rebase me! label Jan 6, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Needs another rebase. :/

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Rebased.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

This is a dependency for my upcoming animation priority system (because that requires getting rid of ReplaceAnim in WithMoveAnimation), and kind of for adding the long-requested turret and barrel equivalents of WithMoveAnimation (as it would be a waste of work to base them on bleed's approach).

@reaperrr reaperrr added this to the Next + 1 milestone Mar 4, 2019

@pchote
Copy link
Member

left a comment

A couple of ideas to try and reduce the chance of future regressions:

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch 2 times, most recently from e89bedf to 1dcb875 Mar 7, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Updated.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from 1dcb875 to 3b9b0f8 Mar 8, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

This fails to compile, now.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from 3b9b0f8 to aaf8c84 Mar 8, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Updated again.

  • Fixed compile issues (pushed too early)
  • Made the Update methods in WithMoveAnimation and GrantConditionOnMovement more readable
  • Updated Aircraft as mentioned in #15675 (comment)

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from b75acf1 to f09f790 Mar 18, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

OpenRA.Mods.Common/Traits/Conditions/GrantConditionOnMovement.cs:L59: [SA1119] The line contains unnecessary parenthesis.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from f09f790 to 35b6288 Mar 18, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Fixed.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Updated, polished the code and wrote update rule.

@obrakmann
Copy link
Contributor

left a comment

Looks ok to me. Didn't spot any in-game regressions. 👍

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from c1b9df4 to ee301a5 Mar 24, 2019

@pchote
Copy link
Member

left a comment

A few notes on how to simplify the bitwise logic:

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

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch 2 times, most recently from 87599fe to 8daeba1 Mar 26, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

Updated.

@pchote
Copy link
Member

left a comment

Just two final comments, otherwise LGTM

reaperrr added 4 commits Aug 26, 2018
Add plumbing for notifying traits of movement
More precisely, about start and stop of movement.
Adapt WithMoveAnimation to INotifyMoving
And add ValidMovementTypes for configurability.

@reaperrr reaperrr force-pushed the reaperrr:notify-moving branch from 8daeba1 to 0e722fe Mar 30, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 30, 2019

Updated.
I've tested the changes to the update rule too, works as intended.

@pchote
pchote approved these changes Mar 30, 2019

@pchote pchote merged commit c828882 into OpenRA:bleed Mar 30, 2019

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:notify-moving branch Apr 4, 2019

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.