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

[WOC] Implement Court of Locthwain #10973

Merged
merged 1 commit into from
Oct 6, 2023

Conversation

Susucre
Copy link
Contributor

@Susucre Susucre commented Aug 23, 2023

So in this WIP, it is not possible to choose to cast for free. Commenting out "you may play" part:

CardUtil.makeCardPlayable(
        game, source, card, Duration.EndOfGame,
        true, controller.getId(), null
);

does unlock the ability to "you may cast without paying manacost". So I think there is an issue with two competing AsThoughEffect not letting the player choose the appropriate one.

@github-actions
Copy link

Unable to retrieve information for "Court of Lochtwain"

@github-actions
Copy link

Court of Locthwain - (Gatherer) (Scryfall) (EDHREC)

{2}{B}{B}
Enchantment
When Court of Locthwain enters the battlefield, you become the monarch.
At the beginning of your upkeep, exile the top card of target opponent's library. You may play that card for as long as it remains exiled, and mana of any type can be spent to cast it. If you're the monarch, until end of turn, you may cast a spell from among cards exiled with Court of Locthwain without paying its mana cost.

@Susucre Susucre force-pushed the court-of-locthwain branch from 2194190 to 24fbc85 Compare August 31, 2023 14:16
@Susucre
Copy link
Contributor Author

Susucre commented Sep 1, 2023

Ok this PR is a mess, but in the end it works.
Will split the game engine improvement and make PR for them separatly.

@Susucre Susucre changed the title (WIP) [WOC] Implement Court of Locthwain [WOC] Implement Court of Locthwain Sep 4, 2023
@Susucre Susucre marked this pull request as ready for review September 4, 2023 21:08
@Susucre
Copy link
Contributor Author

Susucre commented Sep 4, 2023

I consider Court of Locthwain working with the rework of AsThough effects (I did test it live, and with unit tests). I'll look into some of the known bugs of AsThough and see if I can also make them work from this point in other PRs.

@xenohedron
Copy link
Contributor

Can you describe what you've changed about AsThoughEffect in this PR?

  1. Is it all interdependent or are there separable refactor and logic change?
  2. Does it help resolve [WIP] Allows player to choose which asThough ability to use #9268 at all?
  3. Any impact to REFACTOR: AsThoughEffect need a refactor #9521 or [WIP] Refactor AsThoughEffect to split .applies into two functions #9559?

@Susucre
Copy link
Contributor Author

Susucre commented Sep 5, 2023

Can you describe what you've changed about AsThoughEffect in this PR?

  1. Is it all interdependent or are there separable refactor and logic change?
  2. Does it help resolve [WIP] Allows player to choose which asThough ability to use #9268 at all?
  3. Any impact to REFACTOR: AsThoughEffect need a refactor #9521 or [WIP] Refactor AsThoughEffect to split .applies into two functions #9559?
  1. In short, yes. Aside from the Court of Locthwain code and the ConditionalOneShotEffect text generation all the changes should be done together. But since I did add unit tests on Court of Locthwain, I am not in favor of having a 26 file PR, then Court being added.
  2. It should. I'll live test those and report back results, and can add further Unit Test covering those situations.
  3. No progress on those. Applying AsThoughEffect does have side effects of setting info in the Player's state.

@Susucre
Copy link
Contributor Author

Susucre commented Sep 5, 2023

So 2. let the user choose, but I need to add some MageIdentifier restriction from the alternative cost of Bolas's Citadel or else it does apply to Vivien's alternative cast too. I'll make another PR for that.
image

And I do have a way to make RisenExecutioneer cost reduction only apply for its own alternative cast, simply by introducing a specific MageIdentifier for it and setting the cost reduction to only apply on that MageIdentifier alternative cast.

@xenohedron
Copy link
Contributor

Okay, I'm fine with the card and unit tests in this PR with the rework then, and the other PR #11114 to fix those old bugs can be rebased after this is merged. The other issue of the side effects in applies method can be revisited separately.

Please address all feedback from #11114, and update PR title and description to clearly explain what is being changed and why.

@Susucre Susucre marked this pull request as draft September 8, 2023 16:28
@Susucre
Copy link
Contributor Author

Susucre commented Sep 8, 2023

Current plan is to rework the effects in #11114, then I'll rebase this PR with it.

@Susucre Susucre force-pushed the court-of-locthwain branch from 49d5cf5 to 086abd4 Compare October 3, 2023 09:17
@Susucre Susucre marked this pull request as ready for review October 3, 2023 09:17
@Susucre
Copy link
Contributor Author

Susucre commented Oct 3, 2023

Rebased now that the AsThough rework has been merged. Everything works well.

public boolean apply(Game game, Ability source) {
Player controller = game.getPlayer(source.getControllerId());
Player opponent = game.getPlayer(getTargetPointer().getFirst(game, source));
if (controller == null || opponent == null || source == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

(no need to null check source; indeed you've already called getControllerId() on it at this point)

@xenohedron xenohedron merged commit cf395f9 into magefree:master Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants