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 overlapping infantry and building decorations #15147

Merged
merged 8 commits into from Jun 2, 2018

Conversation

Projects
None yet
6 participants
@pchote
Copy link
Member

pchote commented May 19, 2018

This PR polishes our overlay decorations for infantry and structures. The repair and powerdown icons are now shown side by side in RA/D2K/TS, and the heal/tiberium protection/veterancy icons now alternate on infantry where there is not enough space to do anything else.

This also adds the hospital heal indicator to RA, and changes it to blink and only display when the infantry is actively healing. The hazmat indicator in TD is similarly only shown when the infantry is in a tiberium field.

Supersedes #14810.
Closes #14736.

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented May 19, 2018

Fixed the code style warnings.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 19, 2018

The yaml syntax looks really confusing on first sight.

WithDecoration@RANK-1:
BlinkInterval: 32
BlinkPatterns:
hospitalheal: True, False

This comment has been minimized.

@reaperrr

reaperrr May 19, 2018

Contributor

This is really bound to confuse modders. In my opinion it would be more modder-friendly to introduce a Blink enum with On/ Off and map that to True/False internally.

This comment has been minimized.

@pchote

pchote May 20, 2018

Author Member

What if I rename the fields to VisibilityInterval, VisibilityPattern, VisibilityPatterns instead?

This comment has been minimized.

@GraionDilach

GraionDilach May 20, 2018

Contributor

Wouldn't help, Blink sounds more reasonable here. I grasped the syntax now and tbh I don't think renaming this would help. It needs more detailed descriptions IMO.

@pchote pchote force-pushed the pchote:blink-decorations branch from 0db0634 to 7215764 May 20, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented May 20, 2018

Added blink On/Off enum.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 27, 2018

I would open an issue after this PR gets merged to refactor RepairableBuildings to utilize the stacked conditions for the repair effect as well.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍 otherwise.

@pchote pchote force-pushed the pchote:blink-decorations branch from 7215764 to 1b05bc5 May 28, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented May 28, 2018

Rebased.

@Smittytron
Copy link
Contributor

Smittytron left a comment

Looked good in testing

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Jun 2, 2018

Needs another rebase, LGTM after that.

@pchote pchote dismissed stale reviews from reaperrr and Smittytron via 3df0741 Jun 2, 2018

@pchote pchote force-pushed the pchote:blink-decorations branch from 1b05bc5 to 3df0741 Jun 2, 2018

@pchote pchote removed the PR: Rebase me! label Jun 2, 2018

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Jun 2, 2018

Rebased.

@@ -53,7 +53,8 @@ public class UpdatePath
new RemovePaletteFromCurrentTileset(),
new DefineLocomotors(),
new DefineOwnerLostAction(),
new RenameEmitInfantryOnSell()
new RenameEmitInfantryOnSell(),
new SplitRepairDecoration(),

This comment has been minimized.

@pchote

pchote Jun 2, 2018

Author Member

I've deliberately added a comma here in the hopes that it will reduce the rebase conflicts going forward.

This comment has been minimized.

@reaperrr

reaperrr Jun 2, 2018

Contributor

Makes sense, I'll try to keep that in mind for my PRs as well.

@reaperrr reaperrr merged commit a61454f into OpenRA:bleed Jun 2, 2018

2 checks passed

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

@pchote pchote deleted the pchote:blink-decorations branch Jul 9, 2018

@dragunoff

This comment has been minimized.

Copy link
Contributor

dragunoff commented Aug 13, 2018

I think that the blinking hospital indicator is a bit jarring. IMO it should be simply displayed, without blinking when infantry is healing. If it is to blink, then I'd suggest making it smoothly pulse by animating the opacity of the overlay.

I'm not sure whether I should file a new issue for that, just my opinion here.

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.