Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DSK] Implement Omnivorous Flytrap #13083
Changes from 2 commits
a6e95c4
22c3f5c
f7b0fa1
c2713ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)