Skip to content

Pass order name to OrderEffects#21516

Merged
pchote merged 1 commit intoOpenRA:bleedfrom
towelcoded:OrderEffectsScope
Oct 5, 2024
Merged

Pass order name to OrderEffects#21516
pchote merged 1 commit intoOpenRA:bleedfrom
towelcoded:OrderEffectsScope

Conversation

@towelcoded
Copy link
Contributor

@towelcoded towelcoded commented Aug 2, 2024

Pass the order name to OrderEffects (until now, only the target was passed) to allow mods to scope their custom order effects to specific orders.

Required by OpenE2140 for the attack flash animation.

@pchote
Copy link
Member

pchote commented Aug 2, 2024

IMO this is making the upstream trait needlessly complicated, and it would be better for E2140 to simply implement its own trait that implements INotifyOrderIssued (with us taking the interface tweak from the other PR).

@towelcoded
Copy link
Contributor Author

IMO this is making the upstream trait needlessly complicated, and it would be better for E2140 to simply implement its own trait that implements INotifyOrderIssued (with us taking the interface tweak from the other PR).

So do we keep the scoping here, or should I move everything except passing the order name to OrderEffects to our custom trait?

@pchote
Copy link
Member

pchote commented Aug 4, 2024

My preference would be moving everything except passing the order name to OrderEffects to your custom trait

Pass order name to OrderEffects
@towelcoded towelcoded changed the title Add scope to OrderEffects, make OrderIssued extensible Pass order name to OrderEffects Aug 4, 2024
Copy link
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.

I think this PR made sense (before the last push). A filter for orders is a good addition, but instead of a whitelist I think having both a whitelist and blacklist would be better. Then you could easily duplicate the traits for various effects

@pchote pchote merged commit 6794b2d into OpenRA:bleed Oct 5, 2024
@pchote
Copy link
Member

pchote commented Oct 5, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants