Card: move Adventure and Omen to getAllPossibleAbilities

[Hanmac]

Now Card.updateSpellAbilities is clean from other shenanigans

[Hanmac]

@tool4ever is this ready?

[tool4ever]

no, I doubt it
this one seems way riskier than your previous #11654

have you even audited any one the many places where this gets used if they're now missing adventures? (ignore desktop, tests would complain)

[Hanmac]

@tool4ever shouldn't be a problem, there is nothing else looking for "isAdventure"

[tool4ever]

look I really don't have the time to do all this testing for you, especially when it feels like I have to proof my concerns first 😩

So here's my last input, after that is fixed someone else should check a few other cases:

Adventure spell is now unavailable

[Hanmac]

@tool4ever i updated getSpellsFromPlayEffect, so it should work again

[tool4ever]

yea I can confirm
guess I'll try to test the other 51 call sites during the weekend (if the weather isn't too nice)
also need to check #11700 for merge first probably

[tool4ever]

There are even more because some call getSpellAbilities or getNonManaAbilities on CardState directly

also this:

and getBasicSpells

hasSpellAbility seems safe

[Hanmac]

FirstSpellAbility should be the Intrinsic one anyway, not from a different CardState

[tool4ever]

adventure details text is now broken ingame

[Hanmac]

adventure details text is now broken ingame

more details:
Desktop or Mobile?

[tool4ever]

well both

[Hanmac]

You mean "Card Details" or "Card Picture"?

like they are shown in Card Picture:

and should not be shown in Card Details

IMO Prepared should not have been shown there, but you rejected #11634

[tool4ever]

it worked before, with your PR it doesn't

[Hanmac]

it worked before, with your PR it doesn't

You should read my post.

and should not be shown in Card Details

[tool4ever]

of course it needs to be shown in the GUI somewhere 👎

[Hanmac]

of course it needs to be shown in the GUI somewhere 👎

Like I said above, it is shown the "Card Picture" just not in "Card Details"

[tool4ever]

no, picture and details are two different tabs and can be used differently or only one of them

[Hanmac]

@tool4ever should be fixed

[tool4ever]

sigh, now it's displayed all the time instead which also means it looks broken in deck editor

[Hanmac]

sigh, now it's displayed all the time instead which also means it looks broken in deck editor

It's the same with Prepared, that is also displayed all the Time

You should make up your mind what you want

[tool4ever]

Well Prepared is slightly different since it can still be relevant when the host is on battlefield
could only show it when it's unprepared though

[Hanmac]

@tool4ever Prepared was broken in Deck Editor before, so that is not my fault

[Hanmac]

@tool4ever @kevlahnota anything else GUI related is OUTSIDE of this MR

[tool4ever]

I'm just asking you not to break existing stuff and like I said this is your PR doing it 🤷‍♂️

[Hanmac]

I'm just asking you not to break existing stuff and like I said this is your PR doing it 🤷‍♂️

the exact same thing is broken for Prepared too, but i don't see you complain there, or try to fix it

DeckEditor doesn't have AbilityText yet

THE MAIN PROBLEM is that in-game GUI is used for outside of game

[Hanmac]

I'm just asking you not to break existing stuff and like I said this is your PR doing it 🤷‍♂️

what if i REMOVE the Description again and the user can use the SWIRL Icon instead?
Would that make you HAPPY?!

[tool4ever]

look, sorry if I'm making you upset I'm mostly just trying to keep consistency 🙏

yes Prepared is affected already and that seems to be caused by #11635 too
so maybe let's not repeat that before understanding how GUI/ingame texts get split (I agree it looks somewhat messy)

I think that Swirl was originally created for DFC like on Scryfall, but for some reason deck editor now uses it for other AltState printed on same face too 🤔

[tool4ever]

@Hanmac
id is -1, can you check for that maybe?

[Hanmac]

but for some reason deck editor now uses it for other AltState printed on same face too 🤔

it kinda makes sense if you want to see only the Alternate State

These choices:

• i could leave this MR as it is, and we need to fix the AbilityText for another MR (something doesn't call updateAbilityText for the DeckEditor, and it wasn't your MR, i checked)
  This also might break some more stuff, because it might not be exprected to be called outside of game
• I remove this Description because IMO it shouldn't be on this CardState if we keep the Alt Swirl
• I remove this Description if it is outside of Game, this would keep the Description in Game, but removes the broken DeckEditor one
  I would prefer this one as compromise.
• We refactor the GUI for non-Game to only use OracleText and don't create Game Objects,
  that would be a Bigger MR but we would need to do that anyway.
  This is one of the reasons why DeckEditor PaperCard calls getCardForUi to get a GameCard for rending

[tool4ever]

I tried to remove it but on mobile you can't switch to these altstates so it was missing completely
maybe you can find it instead

[Hanmac]

I would say we keep it in this state, and look at the Swirl on Mobile Later

[Hanmac]

@tool4ever is this MR ready? you still need to revoke your "requested change"

[tool4ever]

no, I don't have time for all the auditing work yet (most of them are pretty obvious tbh)

but here are your getBasicSpells regressions for now:

• RevealAi - misses Miracle/Kefnet playability check, not super common but should be small fix either way
• UntapAi - misses extra pool logic
• PlayerControllerHuman - can no longer cast them via dev mode

[Hanmac]

PlayerControllerHuman - can no longer cast them via dev mode

Fixed

[Hanmac]

RevealAi - misses Miracle/Kefnet playability check

i updated the AI code, removed "MiracleCost" (have it look at the Play Sub, and use the params from there)

i also removed getBasicSpells because it isn't used anymore