Unbound Flourishing's X doubling should be a triggered ability (and related refactors) #12597

ssk97 · 2024-07-23T06:43:19Z

Note the "Whenever" in the rules text, it's a triggered ability and not a replacement effect.

Since Unbound Flourishing was the only card to use the X_MANA_ANNOUNCE event and all the associated multiplier code, that should probably all also be removed as well? Leaving this as a draft while I figure out what should be removed there.

edit: now also include a rework of "copy that ability" effects to use the stack/LKI instead of a saved object

JayDi85 · 2024-07-23T07:39:39Z

Mage.Sets/src/mage/cards/u/UnboundFlourishing.java

+        Spell spell = game.getStack().getSpell(event.getTargetId());
+        if (spell != null && spell.isPermanent(game)) {
+            if (spell.getSpellAbility().getManaCostsToPay().containsX()) {
+                game.getState().setValue(this.getSourceId() + UnboundFlourishing.needPrefix, spell);


Make sure it support multiple triggers/spells. Current code save only one spell per Unbound Flourishing.

Good point. I copied it from the copy spell/ability trigger, both of them need to be fixed in that manner.

Also don't forget about flourishing ZCC in setValue. So blinked Unbound Flourishing will ignore outdated spell data.

A better solution is to not use the global game state, and instead use target pointers or the effect value map.

Mage.Sets/src/mage/cards/u/UnboundFlourishing.java

…pointer activated ability triggers

…shing

ssk97 · 2024-07-27T07:24:52Z

Haven't done as much testing with the changes as I'd like so probably best not to merge quite yet, but I think this is all good now.

xenohedron · 2024-08-13T03:57:16Z

Mage/src/main/java/mage/game/stack/SpellStack.java

@@ -82,6 +83,7 @@ public boolean counter(UUID objectId, Ability source, Game game, PutCards putCar
            if (!game.replaceEvent(GameEvent.getEvent(GameEvent.EventType.COUNTER, objectId, source, stackObject.getControllerId()))) {
                if (!(stackObject instanceof Spell)) { // spells are removed from stack by the card movement
                    this.remove(stackObject, game);
+                    game.rememberLKI(Zone.STACK, stackObject);


interesting

This is required by the test I added - previously it was storing the entire object inside the effect which made it ok to not store it in LKI, but now that I changed it to use a FixedTarget I needed the LKI there.

xenohedron · 2024-08-13T03:58:17Z

Mage/src/main/java/mage/abilities/meta/OrTriggeredAbility.java

            if (ability.checkEventType(event, game) && ability.checkTrigger(event, game)) {
                toRet = true;
            }
+            ability.getEffects().clear(); //Remove afterwards, ensures that they remain synced even with copying


Though this feels a bit weird, I think I see what this is trying to do and why it can't be done otherwise

That’s code with effects copy must be removed 100%. Conditional abilities from “or” parts must have all real effects. If something put effects in “or” ability (e.g. abilities container) then that’s wrong logic/design.

Any triggered ability that has SetTargetPointer needs this code to function correctly, as otherwise that inner ability has no access to the effects it's trying to set the target pointers of.

edit: It's also important to note that this isn't copying the effects, it's sharing the effects that are currently inside the OrTriggeredAbility.

It’s weird. Whole OrTriggeredAbility is big workaround with potentially buggy code (example: if something clear/reset data inside triggered ability or init something then it will be broken with that “container”). Must be reworked/removed someday to simple ability with multiple conditionals.

So new part for temporary add/remove is “fine” here — it’s a fix for existing logic.

Only 53 usages of OrTriggeredAbility. How many of them need target pointers set in order to function? Seems like the sort of thing that could be excluded from scope, with those few usages getting cleaner custom implementations

JayDi85

@ssk97 must be improved, also contains suspected code (see comments).

Mage/src/main/java/mage/abilities/effects/common/CopyStackObjectEffect.java

JayDi85 · 2024-08-22T21:04:16Z

Mage/src/main/java/mage/abilities/meta/OrTriggeredAbility.java

            if (ability.checkEventType(event, game) && ability.checkTrigger(event, game)) {
                toRet = true;
            }
+            ability.getEffects().clear(); //Remove afterwards, ensures that they remain synced even with copying


That’s code with effects copy must be removed 100%. Conditional abilities from “or” parts must have all real effects. If something put effects in “or” ability (e.g. abilities container) then that’s wrong logic/design.

Mage/src/main/java/mage/filter/predicate/other/AbilitySourceAttachedPredicate.java

JayDi85 · 2024-08-22T21:09:27Z

Mage/src/main/java/mage/filter/predicate/other/NotManaAbilityPredicate.java

+        if (!(input instanceof Ability)) {
+            return false;
+        }
+        return !((Ability) input).isManaAbility();


Looks strange. Mana abilities never goes to stack due mtg rules. So it’s useless to search stack objects for it.

Huh, that's a good point. If I understand correctly, that means that FilterActivatedOrTriggeredAbility just automatically includes the "not a mana ability" clause by its very design?

Yep, before this it was only being used for targeting

ssk97 · 2024-08-22T21:25:06Z

~~Github's not letting me reply to #12597 (comment), but~~ the purpose of that is that the abilities have code inside of them where the effects' TargetPointer gets set based on what triggered the ability. edit:figured it out

Conditional abilities from “or” parts must have all real effects

The abilities normally will not have any effects, the OrTriggeredAbility has them. However, shortly above that removal, I temporarily add those effects to the ability so that they can set any target pointers necessary.

Unbound Flourishing's X doubling should be a triggered ability

e9bfd92

github-actions bot added cards tests labels Jul 23, 2024

JayDi85 reviewed Jul 23, 2024

View reviewed changes

Mage.Sets/src/mage/cards/u/UnboundFlourishing.java Outdated Show resolved Hide resolved

ssk97 added 3 commits July 23, 2024 15:28

Improve Unbound Flourishing implementation

118db64

Use setTriggerPhrase for most abilities using CopyStackObjectEffect.

4fcc05a

Remove now-unneeded infrastructure for X multiplier

c7babd6

github-actions bot added server engine labels Jul 23, 2024

ssk97 added 8 commits July 23, 2024 16:05

Fix trigger phrases

8bbbd1b

Add ActivateAbilityTriggeredAbility, additional reworking

fad9fdd

Update all copy ability effects, add NotManaAbilityPredicate, target …

b1744a5

…pointer activated ability triggers

Merge remote-tracking branch 'origin/master' into ReworkUnboundFlouri…

fcc5ec2

…shing

fix pointer, remember ability LKI on removal

ccbe8ba

Merge remote-tracking branch 'origin/master' into ReworkUnboundFlouri…

6bc9b96

…shing

Fix my bad pointer for Mirror Shield Hoplite

248b86a

AbilitySourceAttachedPredicate, fix more bad setTargetPointers

6f6a288

ssk97 changed the title ~~Unbound Flourishing's X doubling should be a triggered ability~~ Unbound Flourishing's X doubling should be a triggered ability (and related refactors) Jul 27, 2024

ssk97 marked this pull request as ready for review July 27, 2024 07:24

ssk97 added 2 commits July 27, 2024 00:26

Remove debug messages

d99c998

Fix wrong filter source

20f3ef8

xenohedron approved these changes Aug 13, 2024

View reviewed changes

ssk97 added 2 commits August 12, 2024 21:32

Remove repeated SetTargetPointer line

7d05886

RingsOfBrighthearth text fix

d91e655

ssk97 merged commit b70638a into magefree:master Aug 22, 2024
4 checks passed

JayDi85 reviewed Aug 22, 2024

View reviewed changes

JayDi85 mentioned this pull request Sep 8, 2024

OrTriggeredAbility must be reworked/removed (Unbound Flourishing and other) #12826

Open

JayDi85 mentioned this pull request Sep 8, 2024

NotManaAbilityPredicate must be removed as useless #12827

Open

JayDi85 mentioned this pull request Sep 25, 2024

LKI of copied spells are not fully implemented #12911

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unbound Flourishing's X doubling should be a triggered ability (and related refactors) #12597

Unbound Flourishing's X doubling should be a triggered ability (and related refactors) #12597

ssk97 commented Jul 23, 2024 •

edited

Loading

JayDi85 Jul 23, 2024

ssk97 Jul 23, 2024

JayDi85 Jul 23, 2024

ssk97 Jul 23, 2024

ssk97 commented Jul 27, 2024 •

edited

Loading

xenohedron Aug 13, 2024

ssk97 Aug 13, 2024 •

edited

Loading

xenohedron Aug 13, 2024

JayDi85 Aug 22, 2024 •

edited

Loading

ssk97 Aug 22, 2024 •

edited

Loading

JayDi85 Aug 22, 2024

xenohedron Aug 23, 2024

JayDi85 left a comment

JayDi85 Aug 22, 2024 •

edited

Loading

JayDi85 Aug 22, 2024

ssk97 Aug 22, 2024

xenohedron Aug 23, 2024

ssk97 commented Aug 22, 2024 •

edited

Loading

Unbound Flourishing's X doubling should be a triggered ability (and related refactors) #12597

Unbound Flourishing's X doubling should be a triggered ability (and related refactors) #12597

Conversation

ssk97 commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssk97 commented Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

ssk97 Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayDi85 Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

ssk97 Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JayDi85 left a comment

Choose a reason for hiding this comment

JayDi85 Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssk97 commented Aug 22, 2024 • edited Loading

ssk97 commented Jul 23, 2024 •

edited

Loading

ssk97 commented Jul 27, 2024 •

edited

Loading

ssk97 Aug 13, 2024 •

edited

Loading

JayDi85 Aug 22, 2024 •

edited

Loading

ssk97 Aug 22, 2024 •

edited

Loading

JayDi85 Aug 22, 2024 •

edited

Loading

ssk97 commented Aug 22, 2024 •

edited

Loading