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 WithHarvesterSpriteBody #16298

Merged
merged 1 commit into from Apr 14, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Mar 10, 2019

And move PrefixByFullness there.
Also put it into Mods.Cnc, as RA is the only shipping mod using this.

One more prerequisite for a more robust implementation of the animation priority system, as having an *Animation trait use ReplaceAnim from Tick is asking for trouble (technically the problem is just shifted to WithHarvesterSpriteBody, but that means there's now only one very specific, rarely used place where this could become a problem, while bleed's WithHarvestAnimation could cause problems on TD-style harvesters with 'normal' sprite bodies as well).

All changes tested, yaml changes produced purely via update rule.

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

@reaperrr reaperrr force-pushed the reaperrr:harv-spritebody branch from 2bda2c5 to ca64af6 Mar 10, 2019

@@ -53,6 +53,12 @@ public class WithSpriteBody : PausableConditionalTrait<WithSpriteBodyInfo>, INot
readonly RenderSprites rs;
readonly Animation boundsAnimation;

// HACK: Needed to split fullness prefix of WithHarvestAnimation to WithHarvesterSpriteBody

This comment has been minimized.

Copy link
@pchote

pchote Mar 11, 2019

Member

Wouldn't it make more sense to merge WithHarvestAnimation into WithHarvesterSpriteBody and avoid the hack?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 14, 2019

Author Contributor

I'm not convinced, because a) that would bring us back to WithHarvestAnimation not being available to 'normal' sprite bodies, and b) actors which only need the harvesting anim and not the prefix logic would again be forced to carry the latter around as well, possibly breaking compatibility with other animation traits.
Besides, it's really only meant as a short-term, temporary hack that I still plan to replace with the priority logic before Next+1.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

Not sure how to go about this right now, and I guess it would be better to focus on the aircraft PRs first. Closing.

@reaperrr reaperrr closed this Mar 23, 2019

@reaperrr reaperrr reopened this Apr 1, 2019

@reaperrr reaperrr force-pushed the reaperrr:harv-spritebody branch 2 times, most recently from 0edc60f to eca6621 Apr 1, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

Reopened and updated.
I think I've found a way to make the entire thing both more compatible and simpler + less hacky:
Instead of normalizing and updating sequences with prefixes, I use Animation.ChangeImage to toggle between entire image sprites instead.
Works pretty well in RA, doesn't need any changes to the WithSpriteBody base class, and thanks to inheritance this actually saves some yaml lines.

@reaperrr reaperrr removed the PR: Rebase me! label Apr 1, 2019

@pchote pchote added the PR: Needs +2 label Apr 14, 2019

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

Needs a rebase.

@reaperrr reaperrr force-pushed the reaperrr:harv-spritebody branch from eca6621 to 29bc6ac Apr 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Rebased.

Add WithHarvesterSpriteBody
And move PrefixByFullness there.
Also put it into Mods.Cnc, as RA is the only shipping mod
using this.

@reaperrr reaperrr force-pushed the reaperrr:harv-spritebody branch from 29bc6ac to 012829c Apr 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

Fixed.

@abcdefg30 abcdefg30 merged commit 0eb0a5a into OpenRA:bleed Apr 14, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

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