Developing Bugs

[elcnesh]

Yeah ok, but are there any cards that remove eg all counters and then care about how many of a specific type were removed? I doubt it, but I'm not sure how to look for it... You could also store a pair of CounterType and int, which is slightly ugly (RememberedObjects becomes a kind of map then) but should do the trick if you extract it correctly.

[Agetian]

Yeah ok, but are there any cards that remove eg all counters and then care about how many of a specific type were removed? I doubt it, but I'm not sure how to look for it... You could also store a pair of CounterType and int, which is slightly ugly (RememberedObjects becomes a kind of map then) but should do the trick if you extract it correctly.

Ok thanks for the advice! I'll explore it a little bit and see if I come up with anything definite!

- Agetian

[friarsol]

Yeah ok, but are there any cards that remove eg all counters and then care about how many of a specific type were removed? I doubt it, but I'm not sure how to look for it... You could also store a pair of CounterType and int, which is slightly ugly (RememberedObjects becomes a kind of map then) but should do the trick if you extract it correctly.

Cards typically only care about one type of counter to avoid confusion, but there are cases where a card can gain abilities (Necrotic Ooze) so they could have more than one type of "Remove all <type> counter" abilities. As far as I can tell, only Vampire Hexmage and AEther Snap can remove all counters of any type, and neither of them interact with the amount.

[Marek14]

What about cards that move counters like Fate Transfer? Those need to remove counters and then put the same amount and type elsewhere.

[Agetian]

It's possible to correct the above-mentioned functionality (which, by the way, definitely goes beyond Give // Take - Delaying Shield is also affected, I'm still testing for what else may be affected) by pushing Pair.of(counterType, i) where "i" is the iteration that is being pushed (into rememberedObjects):

Code: Select all

card.addRemembered(Pair.of(counterType, i));


I couldn't find any instance of code where it would be necessary to unpack this Pair to get the counter type... Cards that I could find all use "Count$RememberedSize" which only accounts for the size of the remembered object set. I made some tests (including cards like Delaying Shield, Give // Take, Lightning Coils, Molten Hydra) and they seem to play well with the change above. Fate Transfer also works (it seems to use a different mechanism for moving counters, by the way). Does anybody know what could possibly need changing with the change mentioned above?

EDIT: Looked through all the calls to getRemembered and ways to utilize "Count$", none of them seem to reference the type of remembered counters (99% of them care about remembered cards and remembered players only, the remaining couple calls care about the size of the set without caring about the contents (such as what is used by Give // Take and Delaying Shield) and also about the number pushed into rememberedObjects as integer). As such, it looks like it should work fine, but please test and improve if necessary and possible (r28732). If this works and if in the future it'll be necessary to refer to the type of remembered counters, it's possible to extract them from the first element of the Pair.

- Agetian

[elcnesh]

Ok, but RememberedSize counts the number of items, so is that the number of tuples or should that be the sum of the right-hand sides of each tuple?

Also, in your commit you use a "1" as the right part of the tuple in one place and "i" in the other. Haven't checked the context, but is that correct?

[Agetian]

Ok, but RememberedSize counts the number of items, so is that the number of tuples or should that be the sum of the right-hand sides of each tuple?

Also, in your commit you use a "1" as the right part of the tuple in one place and "i" in the other. Haven't checked the context, but is that correct?

Yeah it should be the number of tuples, not the sum of the right-hand sides (the "i" is just an iteration number for now to make each entry unique). And yes, "1" was definitely an error, I corrected it to "i" (throwing in Pair.of(type, 1) results in exactly the same issue as with just passing the type - Java probably makes every Pair.of(type, 1) internally use the same object for optimization purposes). Thanks for catching it!

- Agetian

[elcnesh]

swordshine, in r28689 you changed the way ZoneChange Triggers work. I believe this has caused some bugs with triggers running twice or not at all (see viewtopic.php?f=26&t=16670#p171880), which is probably related to the fact that ZC Triggers do not have a specified TriggerZone, since the triggering object is the LKI of the one leaving its zone. I'm not entirely sure how this code behaves or should behave, but I do believe its related to this commit.

[Agetian]

I'm investigating an issue with the cards returning to hand having lingering pre-selected abilities on them, such as e.g. when you play Far // Away and choose Away but cancel it right before paying the mana cost, it returns to your hand and then for the rest of the game you can only cast it as Away (you can't choose Far anymore). This issue seems to also cause e.g. Genju of the Fields to become unplayable after it returns to hand from the graveyard when the enchanted Plains is destroyed.

I have narrowed the issue down to the fact that the card is not fully cleared when it returns to hand on this line (125) in GameAction.java:

Code: Select all

copied = c;


Essentially, if a card comes from the graveyard to hand (or from the stack to hand), it's copied by direct assignment, which seems to carry over all the information from the card including (for some reason) its chosen spell abilities.

Modifying line 125, for instance, this way:

Code: Select all

copied = zoneTo.is(ZoneType.Hand) ? CardFactory.copyCard(c, false) : c;


solves the problem (both the split cards and Genju of the Fields tests pass) because CardFactory.copyCard fully recreates the card from scratch and copies over only the relevant information, skipping the temporary information mentioned above. However, on line 122 there's a comment saying "copy only the cards leaving the battlefield" - so I'm assuming the call above may not be optimal. Does anyone have an idea how to best solve this issue? (it seems rather serious because it affects basically all cards that involve spell ability choice and can then be canceled, as well as certain cards returning to hand which may have lingering spell ability information on them). I'm assuming that calling copyCard there should not be necessary because it's probably only a certain element of cleanup that needs to be performed, but so far I can't identify what exactly needs to be cleared from the card in order for it to return to its normal state without having to fully recreate it with copyCard(c, false).

- Agetian

[elcnesh]

I think basically you should always copy a card (or at least reset its abilities) when it changes zones, which'd also resolve other lingering information issues. That being said, it probably breaks half the cards in the game. I've looked into this once, but haven't found a real solution...

[Agetian]

I think basically you should always copy a card (or at least reset its abilities) when it changes zones, which'd also resolve other lingering information issues. That being said, it probably breaks half the cards in the game. I've looked into this once, but haven't found a real solution...

Yeah, I also thought that would work, but it doesn't in practice (except maybe for cards returning to hand in particular as described above) - the reason is: there's also e.g. the Uba Mask (and friends) issue reported in the bug reports thread, which is basically similar but can't be fixed with the line above. The problem is - if you try to cast a card from exile, then cancel at the moment of mana payment, it goes back to exile and is then uncastable. However, it's impossible to fix this particular issue with a copyCard call because if we use copyCard when the card goes to exile, it loses the relevant information that it's actually castable from exile in the first place, so it becomes completely impossible to cast it (even for the first time)...

- Agetian

[Agetian]

In continuation of my previous investigation, here's a weird thing that I can't understand yet: the Uba Mask issue above does not happen if you add a call to a method that clears the card's triggers to the line mentioned above:

Code: Select all

copied = zoneTo.is(ZoneType.Hand) ? CardFactory.copyCard(c, false) : c;
copied.getCurrentState().clearTriggers();


What I don't understand is - why does it even work this way? I traced through the code and the triggers array in currentState is empty at the moment of the clearTriggers() call. Inside clearTriggers there is no code that would request an update anywhere in the game state or in the view, it merely clears the array (and, actually, because the array is already empty, it merely returns from the method call). However, the game reacts to this "clear trigger" call interestingly by allowing the card to be cast from the exile after a canceled mana payment, while without this (seemingly useless) call it doesn't allow that and the card becomes uncastable. Does anyone know why this is happening? I'm 99.99% sure that it has nothing to do with triggers as such, so I want to get to the bottom of the real issue behind it and hopefully fix it...

- Agetian

[elcnesh]

This surprises me a lot. Although I have to say it does have something to do with triggers: the thing that removes the card from the remembered list of Uba Mask is a static trigger (same for similar cards). That being said, I still wouldn't expect it to work with that added line, unless you somehow clear the triggers of Uba Mask. But that's a whole different story, or does such a thing happen? (ie, does the card actually get removed from the remembered list if you do cast it successfully?)

[Agetian]

This surprises me a lot. Although I have to say it does have something to do with triggers: the thing that removes the card from the remembered list of Uba Mask is a static trigger (same for similar cards). That being said, I still wouldn't expect it to work with that added line, unless you somehow clear the triggers of Uba Mask. But that's a whole different story, or does such a thing happen? (ie, does the card actually get removed from the remembered list if you do cast it successfully?)

Nope, the triggers are not cleared on Uba Mask (and even if you cast the spell successfully, the card that the spell was cast from (from the exile) is still shown on Uba Mask as remembered). The clearTriggers() call is only executed on the card in the exile, and it doesn't clear the triggers on Uba Mask itself...

- Agetian

[friarsol]

Uba Mask should probably only be granting the static trigger to the card that it remembers exactly, including timestamp. I think this is where the main issue is. It just remembers the card by ID since IDs don't change when moving zones.