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

Setup Chronosphere, Iron Curtain and Weather Control Device animations #652

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mailaender
Copy link
Member

@Mailaender Mailaender commented Mar 8, 2020

Depends on OpenRA/OpenRA#17787.

@Mailaender Mailaender added the Requires engine update A new engine version fixes this issue or allows to implement a fix label Mar 8, 2020
@Mailaender Mailaender changed the title Setup Chronosphere and Iron Curtain animations Setup Chronosphere, Iron Curtain and Weather Control Device animations Mar 16, 2020
@sorcerer86pt
Copy link

rebase this please

@Mailaender
Copy link
Member Author

No point in that really as OpenRA/OpenRA#17787 isn't even merged.

@sorcerer86pt
Copy link

HI, the OpenRA/OpenRA#17787 has been merged. I think you can rebase this and retest this

@Mailaender
Copy link
Member Author

No, it is still

ENGINE_VERSION="release-20200202"

@abcdefg30 abcdefg30 removed the Requires engine update A new engine version fixes this issue or allows to implement a fix label Oct 4, 2021
@Mailaender
Copy link
Member Author

Rebased

@MustaphaTR
Copy link
Member

Needs rebase again.

@Mailaender
Copy link
Member Author

Had to rebase again due to #806 going in first.

Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two commits called "Fix resources span multiple sheets."

@penev92
Copy link
Member

penev92 commented Apr 4, 2023

Re using Requires<SupportPowerInfo> - I see that the argument is that we only use that for cases that would crash the game and not cases where otherwise unwanted behaviour would be introduced. That is an upstream convention and while I think it should change I won't hold it against this PR.

@Mailaender
Copy link
Member Author

I think the main purpose is to modify the initialization order, which is not required in case of these rendering traits which are connected via interfaces.

@penev92
Copy link
Member

penev92 commented Apr 4, 2023

Here are some polish points to consider:

  • The Weather Control Device's charged animation should freeze when low on power. IMO it would be best to remove the lightnings, not freeze them, but currently nothing happens, which seems pretty weird.
  • The Nuclear Missile Silo's charged and firing animations play on top of the default one, meaning it is clearly visible underneath! Also the missile sprite has lower ZOffset than the Silo's firing sprite, so the missile comes up from behind the structure when firing.
  • The Chronosphere's animations feel like they play too fast (I think we've mentioned this about other building animations as well some time?), but I'll need to compare to the original. The charged animation should also freeze on low power though.
  • The Iron Curtain's animations play too fast. They should also freeze on low power.

If the animation speed is a general mod thing not specific to these structures it can be addressed in a separate PR.

mods/ra2/mod.yaml Outdated Show resolved Hide resolved
@penev92
Copy link
Member

penev92 commented Apr 10, 2023

Considering how we currently have the new WithSupportPowerChargedOverlay trait play on top of the regular body/animation/overlay trait(s), wouldn't it be better to instead create a trait that grants a condition on support power charged and have that condition enable an overlay (disabling other rendering traits as needed)? This sounds a bit more generic and reusable.

@Mailaender
Copy link
Member Author

From my experience, rendering traits work better with interfaces than loosely coupled with conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants