[don’t merge] Fix and test Rain of Riches

[Susucre]

Fix [[Rain of Riches]] using Predicate logic.

We need to ensure continuous effects are recomputed between the Mana Payment from spell.activate and SPELL_CAST event for this card to work properly. Hence the added game.getState().processAction(game); (if you remove it, 2 of the added tests fail)

Fix #12125
Fix #10347
Fix #9669

[Susucre]

Just to give a little detail on the added processAction(game):

The activate call in PlayerImpl is for Spell::activate, which call for multiple ActivatedImpl::activate.

There you have a call to game.applyEffects at the beginning:

But after payment, there is no reprocess for the effects, hence the watchers/triggers reacting to SPELL_CAST not seeing up to date information.

[alexander-novo]

This is the same fix I used in #9785.

According to Jaydi, there is a problem with adding processAction here.

[Susucre]

Well I have provided an actual reason for the change: the current applyEffect is before any MANA_PAID events are fired, and we need ContinuousEffect to be recomputed after those events and before SPELL_CAST is fired. It could actually be done at the end of one of the activate method instead.

If that is still wrong to do, I'd need an actual situation and a reasoning where it could go wrong.

[alexander-novo]

Yeah that was the same reasoning I provided e.g. here. So you can ask jaydi what's wrong with it, which is something I've been trying to do for a while now.

[JayDi85]

It could actually be done at the end of one of the activate method instead

Adding ApplyEffects to fix something is popular hack and the biggest bad smell. It’s not a real fix in most use cases.

[Susucre]

But it is not an hack here. You have to reapply the continuous effect inbetween mana being paid for casting the spell (since you can not know a Treasure is used before), and the spell being moved to the stack and SPELL_CAST triggers.

The current engine does not have proper game info on Spell Cast firing.

[JayDi85]

I’ll look to it later.

[JayDi85]

You have to reapply the continuous effect inbetween mana being paid for casting the spell

It’s not about reapply single effect. A real game can have hundreds effects. It’s about whole game “reset”: another effects, events, triggers, etc. That’s why it’s a dangerous command.

[Susucre]

There is a call to processAction before each mana being paid on casting spells.
Are you really telling me that the one after the last mana being paid is dangerous (arrow on the one added here)?

[Susucre]

Haven't investigated that one yet, but I hugely suspect #12089 to be the same issue of not processing the last payment properly on activating abilities:

[xenohedron]

I support merging this change.

While indiscriminate usage of game.getState().processAction(game) in the engine is indeed a concern, I think it's pretty clear that it is necessary here in order to support mtg rules. As Susucr points out, it is already being called many times in the sequence of actions to reproduce the bug that would occur without this additional call. I do not see an issue with it. This fix has been waiting for over a year.

[Susucre]

I’ll look to it later.

@JayDi85 so how it's looking?

[JayDi85]

In todo

[xenohedron]

Without any actionable feedback there is no point in delaying this further. There can always be followup improvements.

[Grath]

@JayDi85 please either provide actionable feedback for why one more processAction will destroy XMage as we know it or allow this to be updated to address conflicts and merged.