Conversation
| card.getSpellAbility().addEffect(effect.copy()); | ||
| OverloadedEffect overloadEffect = new OverloadedEffect(effect, target.copy()); | ||
| overloadEffect.setText(effect.getText(card.getSpellAbility().getModes().getMode()) | ||
| .replace("target", "each")); |
There was a problem hiding this comment.
Why you don’t use conditional style effect and target (one pair for single and one pair for all)? Auto-replace looks overdeveloped and potentially buggy.
There was a problem hiding this comment.
The "all" version of the spell needs to have no target when cast. It sounds to me like you're recommending not continuing this rework and keeping the existing code with its fully separate effects.
The whole point of this rework is to remove the duplicated code by auto-replacing the targets.
There was a problem hiding this comment.
Yea, it’s looks strange in current implementation — one shot effect but it can be continues effect inside — thats can broke some effects/targets lifecycle in theory. Not critical and maybe workable.
Also inner effects must support work with target pointers (e.g. each inner effect must have special code to check and use the pointers). Must be carefully replaced and checked.
There was a problem hiding this comment.
I would not call it "special code" as all effects ought to use target pointers as the default, but I agree need to verify each effect properly uses them.
| protected OverloadedEffect(final OverloadedEffect effect) { | ||
| super(effect); | ||
| this.innerEffect = effect.innerEffect; | ||
| this.target = effect.target; |
|
I endorse this direction (just make sure there's adequate test coverage) |
|
I want to add more tests, I'll un-draft this once I do. Aside from that, there are two separate PRs I'll likely be making at some point in the nearish future:
|
|
Mizzix's Mastery - (Gatherer) (Scryfall) (EDHREC)
Vizier of Remedies - (Gatherer) (Scryfall) (EDHREC)
Devoted Druid - (Gatherer) (Scryfall) (EDHREC)
|
|
Only added one more test, but it's an important one that actually fails under the current master. I couldn't see any other tests that would be worth adding, instead I just checked over most of the effects to make sure they properly iterated through the target pointer instead of just grabbing the first. On a side note, I think most of the |
I've thought about this too! Feel free to open an issue to discuss scope seeing as I still haven't gotten around to doing so yet |
3 participants
Casting a spell with Overload converts all "target" text to "each". Currently this is done by having entirely separate effects. Instead, we can apply the effects after setting up a
FixedTargets.I have not yet finished converting all Overload cards, once I do I will remove the old
OverloadAbilityconstructor. Currently only A-D cards have been modified. Several of the more complex Overload cards will require merging the target and overload effect implementations as the target version usesgetFirstTarget.I was wondering if this seemed worth finishing. I don't expect it to fix any bugs, just remove a lot of repetitive code.