-
Notifications
You must be signed in to change notification settings - Fork 785
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
[MKC] Implement Mirko, Obsessive Theorist #11813
[MKC] Implement Mirko, Obsessive Theorist #11813
Conversation
[[Mirko, Obsessive Theorist]] |
Mirko, Obsessive Theorist - (Gatherer) (Scryfall) (EDHREC)
|
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 don't understand what the issue is with the text but I'll take your word for it that this is the best UX solution.
Thanks for the surveil triggered ability refactor.
Don’t use text effect here: its hide original ability and potential text problems with it, and adds new code to support. Its ok to have “strange” text in dialogs. setText is standard code style. Text effects created for additional information or outside abilities/effects. |
Ability ability = new BeginningOfYourEndStepTriggeredAbility( | ||
new ReturnFromGraveyardToBattlefieldWithCounterTargetEffect(CounterType.FINALITY.createInstance()),true | ||
); | ||
ability.setTargetAdjuster(MirkoObsessiveTheoristAdjuster.instance); |
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.
Look at GracefulRestoration - it uses target filter without adjuster. Why not use same way?
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.
Maybe like EarthshakerKhenraPredicate
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.
Verify fail appears to be unrelated to this PR |
I tried hard to get the text to generate nicely but I believe the use of the target adjuster was causing rules generation issues, and setting the rules text with
setText
resulted in excessively verbose instructions when resolving the ability, so I ended up using an info effect to spoof the rules text of the final ability. Other, less heavy-handed approaches all yielded inferior results, so this approach is prioritising UX at the cost of slight code elegance