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

Rain of riches fix #9785

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

alexander-novo
Copy link
Contributor

Fixes #9669.

Please see my comments in the linked issue. This change also makes static abilities checked after a player finishes casting a spell but before watchers and triggers are checked for the SPELL_CAST event.

@@ -1210,6 +1210,7 @@ public boolean cast(SpellAbility originalAbility, Game game, boolean noMana, App
castEvent.setZone(fromZone);
game.fireEvent(castEvent);
if (spell.activate(game, noMana)) {
game.applyEffects();
Copy link
Member

Choose a reason for hiding this comment

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

It's a magic that can ruin game cycle at all. See comments from #8402 for game cycle details and applyEffects/processAction.

game.applyEffects() already called in spell.activate's code

shot_221128_025834

Need more research and possible fix. I don't recommends to add applyEffects for any fixes without critical reason.

Copy link
Contributor Author

@alexander-novo alexander-novo Nov 27, 2022

Choose a reason for hiding this comment

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

I did end up changing this to game.getState().processAction(game); (see last commit). Please let me know if this is acceptable.

game.applyEffects() is already called in spell.activate's code, but this is before costs are paid. Please take a look at my comment in the linked issue (#9669) for details.

@alexander-novo
Copy link
Contributor Author

The latest CI failure seems to be because the snow-covered island in J22 isn't correctly listed as basic

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.

@JayDi85 this one's been waiting for a while, could you explain what needs to be done?

Mage.Sets/src/mage/cards/r/RainOfRiches.java Show resolved Hide resolved
@alexander-novo
Copy link
Contributor Author

I'll check tests on my local computer in the morning with a merged branch just to make sure everything is still working since I made this... wow a year ago.

@xenohedron
Copy link
Contributor

I re-ran the travis build and it passed

@alexander-novo
Copy link
Contributor Author

Does Travis merge with master though? Or just runs tests in the branch itself?

@xenohedron
Copy link
Contributor

It runs a build against the merge commit. You should be able to see the details under the "Checks" tab.

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

Successfully merging this pull request may close these issues.

Rain of Riches does not cascade
4 participants