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

Morph rework and check spell characteristics #11178

Merged
merged 18 commits into from
Oct 3, 2023

Conversation

ssk97
Copy link
Contributor

@ssk97 ssk97 commented Sep 18, 2023

Morph should be a separate SpellAbility rather than an AlternativeSourceCostsImpl. Changing this allows me to support the interaction with [[Liberator, Urza's Battlethopter]] and remove a lot of the code special casing [[Zoetic Cavern]].

Also, changed tests which "Cast X" to require the exact name, instead of just checking the start. Previously it allowed casting with alternative modes without actually naming them, which doesn't seem like good behavior.

Fixes #10168
Fixes #11162
Fixes #7160
Fixes #7121
Fixes #4294
Fixes #7138

Needs more testing and is currently missing some more of the abilities that should be checking spell characteristics rather than the card's, but the basic idea is complete. Completed

This also allows me to convert AlternativeSourceCostsImpl to use CostTags once that change be merged, since Morph doesn't fit well with it as-is.

@jeffwadsworth
Copy link
Contributor

Morph is a special action that doesn't use the stack. Is this covered in your code?

@Grath
Copy link
Contributor

Grath commented Sep 21, 2023

Morph is a special action that doesn't use the stack. Is this covered in your code?

This pull request doesn't change TurnFaceUpAbility (the special action to turn a face-down card face-up), it just changes how a card in your hand with (Mega)Morph turns into a 2/2 face-down creature on the battlefield.

@xenohedron
Copy link
Contributor

I like the direction

@Qumojo
Copy link

Qumojo commented Sep 25, 2023

Not sure if I should add it here or create a new thread but here it goes. When I play a morph card (I am a newbie, learning slowly with my friend) then I can't check the card and some times forget what they all do/are! Could we get an option when moving over the card with the mouse the original side will show up instead of that generic morph? Thanks

@ssk97
Copy link
Contributor Author

ssk97 commented Sep 27, 2023

Manual testing suggests that in gameplay terms my changes function correctly, though I didn't test every card affected by the changes to checking characteristics. However, there are some remaining UI issues.

The creature seems to keep the cast ability while on the battlefield
image
I'll keep this PR in draft mode until I fix this (unless instructed otherwise)

Also, on the "choose how to play card" popup the Morph text is a little long:
image
with the entire reminder text being there if you scroll. Not sure if this is a problem or not.

Could we get an option when moving over the card with the mouse the original side will show up instead of that generic morph?

That's definitely something that should go in a separate issue/pull request. It sounds like a good idea, but would require a good bit of UI work.

@xenohedron
Copy link
Contributor

should fix #7160 ?

@ssk97 ssk97 marked this pull request as ready for review October 2, 2023 00:10
@ssk97
Copy link
Contributor Author

ssk97 commented Oct 2, 2023

Fixed the cast ability (it incorrectly had setWorksFaceDown set) and removed SpellAbilityType.FACE_DOWN_CREATURE since it was only used for Zoetic Cavern and is thus no longer needed.

I believe this PR is now ready for review.

@xenohedron
Copy link
Contributor

Copy link
Contributor

@xenohedron xenohedron left a comment

Choose a reason for hiding this comment

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

Thanks for separating out into logical commits, it really helped streamline the review. Basically, all the changes look good to me. There are a couple minor style things that should be easy to address. The main thing I'd like to see is some sort of documentation to guide future developers into correctly using the spell characteristics check when applicable.

@ssk97
Copy link
Contributor Author

ssk97 commented Oct 2, 2023

Two more issues to check that I think might be resolved by this rework

Second issue is definitely fixed by making morph involved in the characteristics checking.

First issue seems to have two parts. The first is fixed while the second is not. Fixing that I think involves moving the "can't cast" effects from CAST_SPELL_LATE to CAST_SPELL events, but there's probably other bits involved that I haven't investigated.

@JayDi85
Copy link
Member

JayDi85 commented Oct 2, 2023

However, there are some remaining UI issues
The creature seems to keep the cast ability while on the battlefield

If you allows to activate it then it's not GUI issue, it's a critical game engine issue and must be fixed (AI will use it, users can use it too).

Also, on the "choose how to play card" popup the Morph text is a little long with the entire reminder text being there if you scroll. Not sure if this is a problem or not

It's ok

@ssk97
Copy link
Contributor Author

ssk97 commented Oct 2, 2023

If you allows to activate it then it's not GUI issue, it's a critical game engine issue and must be fixed.

It does not. The permanent kept the SpellAbility, but it could only be activated from the hand (or other zones if something lets it be cast from there), same as other SpellAbilitys.

Now without setWorksFaceDown, it doesn't appear at all (though the permanent still has the ability, again same as all SpellAbilitys).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment