Developing Bugs

[swordshine]

r26560, @elcnesh, that fix is not good. It's a general LKI issue. The order should not be reversed. I think it's better to apply a fake Pump effect here.

[elcnesh]

r26560, @elcnesh, that fix is not good. It's a general LKI issue. The order should not be reversed.

I know, but in this case the order really doesn't matter, functionally. But I agree the LKI issue should be fixed... But due to the current structure of looking for defined cards it's very hard. I'll have a look into it.

[swordshine]

I know, but in this case the order really doesn't matter, functionally. But I agree the LKI issue should be fixed... But due to the current structure of looking for defined cards it's very hard. I'll have a look into it.

Nope, the order is important when applying static effects. If a equipment has an ability "Equipped creature has Indestructible." and you destroy the equipment first, the original equipped creature can be destroyed. The LKI issue is complicated, but we can handle those two cards using a fake pump effect and remembering the original equipments attached to the targeted creature.

[elcnesh]

Ah, very good point... Anyway, fixed now.

[elcnesh]

I think I may have found the source of the ConcurrentModificationExceptions... In r25449, the loop in getHiddenExtrinsicKeyword was changed from an index loop to an iteration, causing the while(true) and try...catch(IndexOutOfBoundsException) construction to no longer function. Changing the catch type to ConcurrentModificationException may very well solve it. (I'm not just doing that because I don't entirely understand what's going on, and maybe someone else does.)

[friarsol]

I think I may have found the source of the ConcurrentModificationExceptions... In r25449

Here's a link to the comparison:

http://svn.slightlymagic.net/websvn/com ... ge&compare[]=%2F@25448&compare[]=%2F@25449

[elcnesh]

@swordshine, r26631: Spectra Ward should not not give protection from auras, since the creature is still protected from e.g. any colored auras players want to target at it, or aura abilities that target it.

[swordshine]

@swordshine, r26631: Spectra Ward should not not give protection from auras, since the creature is still protected from e.g. any colored auras players want to target at it, or aura abilities that target it.

Temporarily reverted. I didn't notice the release notes. Looks like we need a total rework of protection, so those hardcoded Flickering Ward can be scripted. IIRC, current structure of protection is also not good for the KeywordEnhancement project.

[swordshine]

Temporarily reverted. I didn't notice the release notes. Looks like we need a total rework of protection, so those hardcoded Flickering Ward can be scripted. IIRC, current structure of protection is also not good for the KeywordEnhancement project.

Here is the list of hardcoded protection effects:

Black Ward
Blue Ward
Cho-Manno's Blessing
Flickering Ward
Floating Shield
Green Ward
Pentarch Ward
Pledge of Loyalty
Red Ward
Tattoo Ward
Ward of Lights
White Ward

I think the exception "This effect doesn't remove sth" is only checked in state-based actions. We may add that exception inside the keyword "Protection", e.g. "Protection:Card.White:Protection from white:[exception](source card of the keyword, maybe use CardUID to identify it)" or "Protection:Card.nonColorless:Protection from all colors:Aura" and tweak stateBasedAction704_5n.

[rikimbo]

I think I may have found the source of the ConcurrentModificationExceptions... In r25449, the loop in getHiddenExtrinsicKeyword was changed from an index loop to an iteration, causing the while(true) and try...catch(IndexOutOfBoundsException) construction to no longer function. Changing the catch type to ConcurrentModificationException may very well solve it. (I'm not just doing that because I don't entirely understand what's going on, and maybe someone else does.)

I was wondering why that catch for IndexOutOfBoundsException was there... Anyway, I think I understand what's going on now.

After looking at the stack trace posted here: http://www.slightlymagic.net/forum/viewtopic.php?f=26&t=14791

It looks like the getHiddenExtrinsicKeyword is getting called by an event handler running on a separate thread. That means that while it is iterating through the class member this.hiddenExtrinsicKeyword, another thread can call a function like removeHiddenExtrinsicKeyword that alters this list.

That's why the catch for IndexOutOfBoundsException was there previously, when it was an indexed loop. If an element was removed from the list by another thread, the size variable of the loop would not be updated, which could cause the loop index to now go out of bounds.

Using ArrayList's iterator will have similar behaviour. The difference is that the iterator will check to see if the structure of the list has changed since the last call to next, so it will throw a ConcurrentModificationException immediately if the size has gone up or down.

So yes, I think that changing the catch block to catch a ConcurrentModificationException will work. It might be kind of a sloppy solution though...

People have suggested using ArrayList's clone method to get around this problem. That is, instead of:

Code: Select all

for (String s : this.hiddenExtrinsicKeyword)

Have this instead:

Code: Select all

for (String s : this.hiddenExtrinsicKeyword.clone())

(You need a cast to ArrayList<String>, but you get the idea.)

As I understand it, the clone method will directly copy chunks of memory to make a shallow copy of the ArrayList object, without using an iterator. Once the copy is made, it's now a separate list from the class member hiddenExtrinsicKeyword, so it is guaranteed that it will not be modified by another thread. This means we can iterate through it safely.

An even more "proper" way to fix this would be to synchronize all accesses to hiddenExtrinsicKeyword, but that's not a one-line fix.

[Farnsworth]

I think I saw somewhere that new contributors are supposed to post patches of commits... is this true? And if so where?

[friarsol]

I think I saw somewhere that new contributors are supposed to post patches of commits... is this true? And if so where?

Just create a new post in the developers corner explaining a little intro of yourself and what your attempting to fix with the patch.

[Chris H.]

I updated to rev 27347 and I now get these two erros:

ClientProtocolJson cannot be resolved to a type NetClient.java /forge-net/src/main/java/forge/net/client line 33 Java Problem

The import forge.net.protocol.ClientProtocolJson cannot be resolved NetClient.java /forge-net/src/main/java/forge/net/client line 11 Java Problem

[Chris H.]

I updated to rev 27347 and I now get these two erros:

ClientProtocolJson cannot be resolved to a type NetClient.java /forge-net/src/main/java/forge/net/client line 33 Java Problem

The import forge.net.protocol.ClientProtocolJson cannot be resolved NetClient.java /forge-net/src/main/java/forge/net/client line 11 Java Problem

 
I deleted the local copy and then re-imported the project and it will now build.

[elcnesh]

I'm working on refactoring block declarations, but in order to do everything correctly, I need the field Card::mustBlock to become a multiset (it needs to count the nr of requirements on each creature to block). However, it uses the new property tracking system which I don't quite fully understand yet. Is there a simple way to convert this?