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

Implement adventures. #6082

Merged
merged 21 commits into from Dec 12, 2019
Merged

Implement adventures. #6082

merged 21 commits into from Dec 12, 2019

Conversation

@phulin
Copy link
Contributor

phulin commented Dec 9, 2019

This PR implements adventures by following a similar approach to that of split cards. This is not ready to merge yet; outstanding issues:

  • Do basic implementation.
  • Add tests.
  • Edgewall Innkeeper, Garenbrig Squire, Wandermare, Mysterious Pathlighter
  • Lucky Clover
  • Memory Theft
  • Fix bug where adventure cast by A but owned by B can't be cast.
  • Fix bug where you can cast adventure from exile.
  • Add rendering code (currently just as extra text like flashback; would welcome additional edits).
  • Add many more tests (copy of copy, player lose/disconnect after adventure exile, multiple adventure cards on battlefield)
  • Ensure correct names are in game log (adventure name on adventure cast, creature name on exiling adventure).
  • Allow casting of Adventures off e.g. Melek, and adventure creatures off e.g. Garruk's Horde, and Adventures from graveyard off W6 emblem.

Due to the way spell-casting is implemented, there is going to be a long tail of bugs and strange interactions. In particular, most effects that allow you to conditionally cast cards from a zone other than your hand (think Precognition Field or Chainer, Nightmare Adept or Muldrotha, the Gravetide) are implemented by examining only the card's characteristics, not the characteristics of each of the cards' abilities. Without significant rearchitecting of a wide swath of the codebase, it will be difficult to track down every one of these bugs.

image

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 9, 2019

If you get SplitCard implementation then you must check and edit (if necessary) all code with SplitCard (search by instanceof SplitCard):

shot_191209_220825

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 9, 2019

If you get SplitCard implementation then you must check and edit (if necessary) all code with SplitCard (search by instanceof SplitCard):

shot_191209_220825

Yep, I've searched and replaced everything that references split cards.

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 9, 2019

Rendering is still very messy

What that mean? You can concat main and second card text in one or use token/copy style (main card with icon on left corner with second card).

P.S. Looks good, if it works then that's can be merged later. BUT you need MORE tests (junit or manually) for special things like copy and copy of copy, cast from opponent hand, player lose/disconnect after adventure exile, counter, multiple adventure cards on battlefield, etc.

phulin added 2 commits Dec 9, 2019
This adds support for Edgewall Innkeeper (and similar cards) and Memory Theft.
@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 9, 2019

P.S. Looks good, if it works then that's can be merged later. BUT you need MORE tests (junit or manually) for special things like copy and copy of copy, cast from opponent hand, player lose/disconnect after adventure exile, counter, multiple adventure cards on battlefield, etc.

I'm happy to add these tests, but not sure exactly what you mean. What needs to be tested after a player loses? And what needs to be tested about multiple adventure cards on the battlefield?

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 9, 2019

As example:

  1. There is singleton effect: you can test 2 adventure cards (e.g. cast one... wait... cast another one -- all effects must be applies and return);
  2. Copy tests from rules:

If an effect copies an Adventure spell, that copy is exiled as it resolves. It ceases to exist as a state-based action; it's not possible to cast the copy as a creature.

If an object becomes a copy of an object that has an Adventure, the copy also has an Adventure. If it changes zones, it will either cease to exist (if it's a token) or cease to be a copy (if it's a nontoken permanent), and so you won't be able to cast it as an Adventure.

  1. Loose/disconnect: if adventure card goes to exile and player loosed/disconnected then game must continue to play (without freeze).

All that use cases is not necessary to test before merge (users will report it by feedback anyway). It's just ideas to implement/fix more potential code (if something missing).

phulin added 4 commits Dec 10, 2019
@phulin phulin changed the title [WIP] Implement adventures. Implement adventures. Dec 10, 2019
@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

Okay, @JayDi85 I think this is almost ready for review. Let me know if there are changes needed - happy to spend more time on this this week. The one remaining issue is that the adventure spell abilities are also showing up on the battlefield, because the AdventureCard getAbilities method just concatenates the two. I'm not sure how to check in the getAbilities or getRules overrides whether the card is on the battlefield - would appreciate any advice you could give.

This prevents the adventure ability text from displaying when the card is on the battlefield.
@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

The latest commit should fix the issue I mentioned above, and with that this is ready to go as far as I'm concerned. Let me know what else you need me to do.

@JRHerlehy

This comment has been minimized.

Copy link
Member

JRHerlehy commented Dec 10, 2019

This is close, but unfortunately is missing some of the interactions of adventures and continuous effects. You did a good job with getting Rule 715.4 to work. However, it is missing support for Rule 601.3e.

601.3e If a rule or effect states that only an alternative set of characteristics or a subset
of characteristics are considered to determine if a card or copy of a card is legal to cast,
 those alternative characteristics replace the object’s characteristics prior to determining 
whether the player may begin to cast it.

Example: Garruk’s Horde says, in part, “You may cast the top card of your library if it’s a 
creature card.” If you control Garruk’s Horde and the top card of your library is a noncreature 
card with morph, you may cast it using its morph ability.

Example: Melek, Izzet Paragon says, in part, “You may cast the top card of your library if 
it’s an instant or sorcery card.” If you control Melek, Izzet Paragon and the top card of your 
library is Giant Killer, an adventurer creature card whose Adventure is an instant named 
Chop Down, you may cast Chop Down but not Giant Killer. If instead you control Garruk’s 
Horde and the top card of your library is Giant Killer, you may cast Giant Killer but not Chop Down.

With Melek in play, you are not presented with the option to cast Adventures. And when you have Garruk's Hord in play, you are incorrectly given the option to cast them for their adventure cost.

This rule also interact with the Emblem you receive from Wren and Six.

You get an emblem with “Instant and sorcery cards in your graveyard have retrace.” 
(You may cast instant and sorcery cards from your graveyard by discarding a land card in addition to 
paying their other costs.)

In this current implementation, you are not presented with the option to retrace your adventures. This is expected per the rules as stated https://twitter.com/elishffrn/status/1178793702568804352?lang=en

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 10, 2019

Found problem with card names and game logs:

shot_191210_131541

  1. Adventure spell cast by parent card name instead adventure name (or parent name + adventure -- (see AwakenAbility as example);
  2. Return to exile by adventure name instead parent name (or parent name + adventure) -- it's can be replaced by custom message like card name exiles and goes to adventure.
@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

Thanks, both. Will fix these issues.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

Hm @JRHerlehy curious if you have thoughts on this - would the same logic apply to Past in Flames? The Eli Shiffrin tweet seems to imply yes.

@emerald000

This comment has been minimized.

Copy link
Contributor

emerald000 commented Dec 10, 2019

Past in Flames give Flashback to Sorcery and Instant cards in your graveyard. Adventure cards are Creatures in the graveyard and thus don't gain Flashback.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

@emerald000 That's true, but the Wrenn and Six emblem does apply despite reading "instant and sorcery cards in your graveyard have retrace."

@emerald000

This comment has been minimized.

Copy link
Contributor

emerald000 commented Dec 10, 2019

That's different though. The emblem is a static effect that always apply. Past in Flames is a one-shot effect that gives an ability to cards while it resolves.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

Fair enough. I'll leave it out for now; as I've edited the comment above to mention, I think there is going to be a long tail of bugs with these that will be difficult to ever completely fix.

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 10, 2019

@JRHerlehy

This comment has been minimized.

Copy link
Member

JRHerlehy commented Dec 10, 2019

Also, I didn’t check how this interacts with Prophet of Kruphix. Given the way Horde is letting you cast adventures from top of library, I’d hazard a guess it’s interacting there too.

@emerald000 I’m correct in assuming that giving flash to creatures doesn’t work with adventures? And Teferi, Time Reveler should let you flash in sorcery adventures, since when the casting restriction is checked, it is a sorcery on the stack?

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 10, 2019

Okay, I've confirmed that sorcery-flash effects like Teferi work - I haven't tested Prophet of Kruphix, either to confirm that flash applies to the creature or that it doesn't apply to the spell.

Wrenn and Six emblem is not currently working with adventures; I'd like to get this PR merged first and then work on getting that done. I think that is not a very common interaction, so not so worried about it for now. I wrote a test for it which is currently ignored.

The text issue has been fixed.
image

Let me know if there are any other issues; aside from what I've outlined above, this is good to go as far as I can tell.

if (sourceObject != null && sourceObject instanceof Card) {
cardToCheckProperties = (Card) sourceObject;
}
}

This comment has been minimized.

Copy link
@JayDi85

JayDi85 Dec 10, 2019

Member

Same changes must be done for all other play the top card cards with same code like Bolass Citadel

This comment has been minimized.

Copy link
@phulin

phulin Dec 10, 2019

Author Contributor

It won't matter for any play the top card cards except those where only some cards are playable (Precognition Field) or where it matters which card is cast (Bolas's Citadel). But I'll take a look through as many as I can.

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 10, 2019

getAbilities of AdventureCard return duplicated abilities (getPlayable methods return duplicates):
shot_191211_031519

It's added on card init:
shot_191211_032149

I'm researched it and found problem -- there are missing override code in AdventureCard:

@Override
    public Abilities<Ability> getAbilities() {
        Abilities<Ability> allAbilities = new AbilitiesImpl<>();        
        allAbilities.addAll(spellCard.getAbilities());
        allAbilities.addAll(super.getAbilities());
        return allAbilities;
    }

If you add it then you can remove spell duplication code in init. All tests works fine with that (except testThatNotCastFromHand test in latest commit).

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 11, 2019

Addition to previous comment.

There were new commit in main repository with wrong rules generation fix -- it's will help with missing adventure text after duplication fix.

What to do:

  1. Merge changes from main repository (or copy code from that commits: 6b5a984 and be7dea2) (CardImpl and PlayerImpl);
  2. Remove spell ability duplication from init code (AdventureCard);
  3. Add missing override method for getAbilities() (AdventureCard);
  4. check both getAbilities methods and put adventure ability first (AdventureCard);
  5. Add support of adventure card in getPlayableObjects (same as split cards, see commit be7dea2) -- it's can fix missing playable marks in theoretic situation: adventure card cost too much, but adventure spell possible to cast).
@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 12, 2019

@phulin do you have any new code for PR? I want to merge it with extra code from my last comment.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Sorry, haven't had a chance to work on it today. Feel free to finish the work I've done if you want to get it merged - I won't be offended.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Actually, have some time to work on it right now

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 12, 2019

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Sure, although some of these changes have caused test failures that I now have to track down. If I can't finish it this evening I'll let you know.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Okay @JayDi85 this commit looks good to merge now if Travis passes, at least from my perspective.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Looks like Bolas's Citadel is broken again. I'll try to fix it and can push in a separate PR.

@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 12, 2019

I have bolas related fixes -- current getPlayable do not support bolas mechanics and you can't use it in tests (but you can use it in normal play).

P.S. Tests fail is ok, you don't need to ignore it before PR merge.

@JayDi85 JayDi85 merged commit 04f7850 into magefree:master Dec 12, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@JayDi85

This comment has been minimized.

Copy link
Member

JayDi85 commented Dec 12, 2019

Thanks for great work on adventure cards.

@phulin

This comment has been minimized.

Copy link
Contributor Author

phulin commented Dec 12, 2019

Excited to see the release so I can no longer have a separate cube list for xmage!

@theelk801

This comment has been minimized.

Copy link
Contributor

theelk801 commented Dec 14, 2019

Thank you so much to everyone who contributed to this, you have no idea how much I appreciate it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.