Add living citadel vosh dm06

[DanieloV]

This adds living citadel vosh (dm06)

To implement this change, changes to the tap ability system were required, so I rewrote it as a condition. The TapAbility is still in 2 parts for every card. The TapAbility attribute on every card now holds the func with the ability, and an fx.TapAbility is added to the c.Use. This also removes the need to mark every card as tapped individually after using its TapAbility.

By implementing Vosh, some cards end up have 2 tap ability options. If that's the case, when using the tap ability they get to choose its source

If they only have one option they are not presented with this screen like before.

A deck for testing purposes of most of the cards affected: { "_id": { "$oid": "66a32d1580d545e1fb155119" }, "uid": "f14866d9-64bc-461c-a00a-8be14b7dc683", "owner": "", "name": "VoshTest", "public": false, "standard": true, "cards": "438*4,414*4,409*4,407*4,429*4,374*4,421*4,456*4,443*4,430*4" }

[sindreslungaard]

I think defining whether or not a card has the tap ability option through conditions makes sense and is a good refactor.

Moving the execution of the actual tap ability from being an event to its own function on the card i'm not so sure about. Looks like the new function has the same signature as an event so could it not be handled like it used to be just with the condition change? This part also moves some logic from individual cards into the creature fx which I think we should try to avoid where possible as it tends to cause problems down the road. E.g. if we want to add a tap ability card that can be used when tapped, since that check is now in the creature fx it is not as easy to do as if that check was on the individual card level

[DanieloV]

This part also moves some logic from individual cards into the creature fx which I think we should try to avoid where possible as it tends to cause problems down the road. E.g. if we want to add a tap ability card that can be used when tapped, since that check is now in the creature fx it is not as easy to do as if that check was on the individual card level

I don't think you can avoid adding stuff to the creature fx. You need a place to handle the situation when a card receives a tap ability from another creature, and that's the only one that I think makes sense unless you have something else in mind. I understand your concern with adding things to the creature part, but unless you have a concrete example of an upcoming card that would totally break things, I don't see a better option.

Also for argument sake, I think the example you give can be easily handled with adding another condition.

Moving the execution of the actual tap ability from being an event to its own function on the card i'm not so sure about. Looks like the new function has the same signature as an event so could it not be handled like it used to be just with the condition change?

Well I cannot have it back to being fx.When(fx.TapAbility as that doesn't work with multiple tap abilities. I can move what's now in the tap_ability file in every single card with tap ability and remove the TapAbility field on the Card struct completely, I just think it looks slightly more clean the way it is now but open to change it if you think it's imperative.

[sindreslungaard]

Yeah you're right about the need of having it in the creature fx or some other shared place, I did not think of the case where you can have multiple of the cards that modifies which tap abilities you can use.

I guess another approach that would work without having it in the creature fx (except the selection part) and as separate functions would be to add an event similar to the PlayCard event where other cards can add themselves as sources for tap abilities similar to how we determine which blockers can be used in cards like Tower Shell. I think that would probably be a bit cleaner as it follows the same pattern that we have established for how attackable creatures and usable blockers are determined and modified, but i'm also ok with this approach.

I'll try to get around to merging your open PRs sometime this week 👍