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
[ICE] Implement Kjeldoran Guard #11184
[ICE] Implement Kjeldoran Guard #11184
Changes from 1 commit
ff6518d
e064ce9
8a0423a
bf4d26a
17cbb8c
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.
Style
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.
Covariant return
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.
Please test- if you flicker Kjeldoran Guard, shouldn't have to sacrifice it.
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.
oh right that should be using MOR
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.
Actually that was already correct.
SacrificeSourceEffect
does sacrificesource.getSourceObjectIfItStillExists(game)
, so the Kjeldoran Guard with a different zcc will not get sacrificed (there still will be a trigger, although it does nothing).I did replace the UUID of the target to a MOR, but that should not really matter with
DelayedTrigger
being set withtriggerOnce=true
.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.
Oh nevermind, there is indeed an issue if you blink the Guard in response to tapping it.
Will rework with a MOR based
SacrificeTargetEffect
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.
Hm, this doesn't feel quite right but not sure why, can you check the delayed trigger displays with correct text on the stack?
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'm not sure we are displaying the trigger when those delayed trigger happens, is that was you feel is not right?
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.
will capitalize
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.
Yeah ideally the trigger phrase should be part of it (but for sure lots of these have wrong text all over the codebase). So not required to fix, just a nice to have.
Also it doesn't get automatically capitalized which is a separate bug.
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 can replace it with "When that creature leaves the battlefield, sacrifice Kjeldoran Guard"?
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.
Should be "When that creature leaves the battlefield this turn, sacrifice {this}."
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.
Just for a discussion point, got a comment from theelk801 last month that reflexive triggers can have their trigger left out. #10866 (comment)
So is that the line? Delayed should have trigger text but not reflexive ones? Or should that be a standalone dev discussion outside of that poor Ice Age common implementation.
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.
That's a good question - my initial reaction is I disagree but I should double check how it works.
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.
It's more important to have clarity on delayed, the reflexive at least occur in close proximity to their source