Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DSK] Implement Omnivorous Flytrap #13083

Merged
merged 4 commits into from
Nov 30, 2024

Conversation

Cguy7777
Copy link
Contributor

No description provided.

@github-actions github-actions bot added the cards label Nov 27, 2024
Mage.Sets/src/mage/cards/o/OmnivorousFlytrap.java Outdated Show resolved Hide resolved
new OmnivorousFlytrapCondition())
.concatBy("Then"));
TargetCreaturePermanentAmount target = new TargetCreaturePermanentAmount(2);
target.setMinNumberOfTargets(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

curious - why is this necessary, was it bugged without it? compare e.g. Armament Corps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I added the line setting a minimum number of targets, the game allowed you to choose zero targets for the ability, which I don't believe is technically allowed?

I just tried [[Armament Corps]], and it lets you choose zero targets.

Looking through open issues, maybe this is what #9457 is about?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I had fixed a separate bug with #11341. I wasn't sure what 9457 was referring to, but this seems like a good guess.

As I understand it, with typical templating, max amounts of 2 or 3 are written as "one or two" or "one, two, or three" implying minimum 1, while higher numbers use "up to" or "any number" implying minimum 0. So I think in TargetAmount constructor, the minimum number of targets should be set using that logic, avoiding the need to specify per card.

Can confirm by then adjusting DamageMultiEffect and DistributeCountersEffect text gen to use description from target pointer rather than hardcoded target. Not sure if any other effect classes in scope? Or if there are any exceptions?

Maybe best as a separate PR.

scryfall search for potentially impacted cards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll start working on a PR for this. For this card, I've switched it back to using the shorter and more typical target code, which will work properly (requiring at least one target) once that future PR is finished.

Copy link
Contributor

@xenohedron xenohedron Nov 30, 2024

Choose a reason for hiding this comment

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

some exceptions: https://scryfall.com/search?q=o%3A%22among+up+to+t%22&unique=cards&as=grid&order=released

would you mind writing up an issue (either fresh or taking over 9457) to describe your intended scope? I'm thinking we might want a TargetAmount constructor that specifies, but open to various approaches. I have some unrelated cleanup to TargetAmount classes and DistributeCountersEffect constructors that I'll try to push soon (edit: pushed)

@xenohedron xenohedron merged commit 1efa094 into magefree:master Nov 30, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants