[don't merge] [LCC] Implement Paleontologist's Pick-Axe and changes of layer system (transform)#11677
[don't merge] [LCC] Implement Paleontologist's Pick-Axe and changes of layer system (transform)#11677DominionSpy wants to merge 3 commits into
Conversation
JayDi85
left a comment
JayDi85
left a comment
There was a problem hiding this comment.
Miss of detailed reason and description, cards and use cases examples, tests -- all that should be given for such code changes.
|
|
||
| // As Sanctuary Blade becomes attached to a creature, choose a color. | ||
| this.addAbility(new AttachedToCreatureSourceTriggeredAbility(new ChooseColorEffect(Outcome.Benefit), false)); | ||
| this.addAbility(new AsAttachesToCreatureAbility(new ChooseColorEffect(Outcome.Benefit), "choose a color.")); |
There was a problem hiding this comment.
Must keep name convention for abilities:
- name part (looks like a rules);
- scope part (Source, All, Target);
- class type (TriggeredAbility, AttachedAbility, ActivatedAbility, etc).
So new name must be like: BecomesAttachedToCreatureSourceTriggeredAbility.
For future refactor: there are few cards with same rules, so it can be shared between multiple cards
There was a problem hiding this comment.
This is not a triggered ability (at least in the rules), so should the name be something like: BecomesAttachedToCreatureSourceAbility?
Re: refactor:
The "whenever" or "when" rules above will cause a trigger, whereas the "as" ones will not. That is the only difference between the rules.
| import mage.game.stack.StackObject; | ||
| import mage.players.Player; | ||
|
|
||
| public class AttachesToEffect extends ReplacementEffectImpl { |
There was a problem hiding this comment.
Must make ref that it was copied from EntersBattlefieldEffect
| activeLayerEffects = getLayeredEffects(game); | ||
| } | ||
|
|
||
| layer = filterLayeredEffects(activeLayerEffects, Layer.CopyEffects_1); |
There was a problem hiding this comment.
Deep game engine changes like that (without tests and detailed description in code and PR) smells very bad.
Why you need all that code instead usage of existing one (there are many other implementations with same "becomes attached to"). Even [[Sanctuary Blade]] already existed and worked in old code.
There was a problem hiding this comment.
Thanks for the feedback.
This change is because the copy effect needed for [[Dinosaur Headdress]] is currently in the same layer as the transform effect. Since there is only one pass of that layer, the copy effect (DinosaurHeaddressEffect) linked with the card on the battlefield does not get added to the layered effects until after the transform effect is processed.
The active layered effects are then re-populated before processing layer 2. This is why non-copy effects work on existing transformed equipment.
[[Sanctuary Blade]] is not the reason for the engine change. However, it currently causes a trigger when it is attached to a creature. This is not correct - it should not allow a response before resolution of the choice.
|
|
||
| TransformEffect() { | ||
| super(Duration.WhileOnBattlefield, Layer.CopyEffects_1, SubLayer.CopyEffects_1a, Outcome.BecomeCreature); | ||
| super(Duration.WhileOnBattlefield, Layer.TransformEffects_0, SubLayer.NA, Outcome.BecomeCreature); |
There was a problem hiding this comment.
See above, maybe related to copy effects rework from #10801 by @xenohedron but you don't write any reasons and details.
JayDi85
changed the title
JayDi85
changed the title
Reasoning for ChangesChanges to Layer System |
xenohedron
changed the title
xenohedron
left a comment
xenohedron
left a comment
There was a problem hiding this comment.
Lots happening in this PR - hard to review.
I recommend a separate PR for fixing Sanctuary Blade with new AsBecomesAttachedSourceAbility first, if that change is independent, including unit test for it.
Then open separate issue explaining the engine limitation preventing Dinosaur Headdress from working, and we can discuss best path for workaround.
| * @author North | ||
| */ | ||
| public enum Layer { | ||
| TransformEffects_0, |
There was a problem hiding this comment.
This does not conform to 613.1 (interaction of continuous effects / layer system). If the workaround is necessary, needs clear explanation of the shortcomings of current mage engine implementation. Maybe related to 701.28 or 712.8e? Is the problem that defining TDFC characteristics shouldn't be a layer 1 effect in the first place?
|
For info: looks like it can be PhysicalEffects_0 layer instead TransformEffects_0 to simulate all physical actions with real life cards like transform, meld, flip, morph/facedown, etc. (maybe mutate can use it too someday). Current logic uses multiple workarounds in many places and it big pain (bug example with face down: #11733 (comment), also see related topic from a copy problems #10801) |
Morph/facedown applies in layer 1b, not the same. 613.2b. Layer 1b: Face-down spells and permanents have their characteristics modified as defined in rule 708.2. |
4 participants
#11239