Skip to content

Add an Offset field to WithDamageOverlayInfo#21116

Merged
PunkPun merged 1 commit intoOpenRA:bleedfrom
abcdefg30:withDamageOffset
Oct 31, 2023
Merged

Add an Offset field to WithDamageOverlayInfo#21116
PunkPun merged 1 commit intoOpenRA:bleedfrom
abcdefg30:withDamageOffset

Conversation

@abcdefg30
Copy link
Copy Markdown
Member

Closes #21113.

@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Oct 12, 2023

Shouldn't the offset really be random?

@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Oct 12, 2023

Something like #19238

@abcdefg30
Copy link
Copy Markdown
Member Author

Shouldn't the offset really be random?

Unless there is a specific use-case requested I wouldn't make this more complicated than necessary. If anything the trait should be made conditional and then you can emulate the behavior even better by using multiple instances.

{
[Desc("Renders an overlay when the actor is taking heavy damage.")]
public class WithDamageOverlayInfo : TraitInfo, Requires<RenderSpritesInfo>
public class WithDamageOverlayInfo : TraitInfo, Requires<RenderSpritesInfo>, Requires<BodyOrientationInfo>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't body orientation be optional?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can make it optional, but that trait does not make sense without it. (All our actors have body orientation.) WithIdleOverlay also requires it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Adjusted it.

{
[Desc("Renders an overlay when the actor is taking heavy damage.")]
public class WithDamageOverlayInfo : TraitInfo, Requires<RenderSpritesInfo>
public class WithDamageOverlayInfo : TraitInfo, Requires<RenderSpritesInfo>, NotBefore<BodyOrientationInfo>, IRulesetLoaded
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the proper way we check is by delaying the acquisition of the trait to INotifyCreated

Copy link
Copy Markdown
Contributor

@michaeldgg2 michaeldgg2 Oct 17, 2023

Choose a reason for hiding this comment

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

Or using Requires? That way the check in RulesetLoaded is redundant. But yes, if this trait doesn't absolutely require a trait, it should use INotifyCreated to retrieve a reference to it.

@abcdefg30
Copy link
Copy Markdown
Member Author

Updated.

Copy link
Copy Markdown
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

LGTM

@PunkPun PunkPun merged commit b35b560 into OpenRA:bleed Oct 31, 2023
@PunkPun
Copy link
Copy Markdown
Member

PunkPun commented Oct 31, 2023

Changelog

@abcdefg30 abcdefg30 deleted the withDamageOffset branch November 1, 2023 00:08
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.

Add Offset for WithDamageOverlay trait

3 participants