Bug Reports (snapshot builds)
Moderators: timmermac, Blacksmith, KrazyTheFox, Agetian, friarsol, CCGHQ Admins
Re: Bug Reports (snapshot builds)
by Agetian » 05 Jan 2017, 19:16
@ pfps: Please take a look at r32947, I made some modifications to help mitigate some of the effects you're mentioning:
http://svn.slightlymagic.net/websvn/rev ... &peg=32980
At least the part with Evolve triggers and stuff. They are now being processed internally together with the IDs of cards they come from, so two evolve triggers from different creatures should not automatically order as if they were the same.
Granted, this change does not help with the Amulet of Vigor problem. Can you please single out your specific proposal for that part of the issue and provide the code that you believe would fix it?
Thanks in advance for your input!
- Agetian
http://svn.slightlymagic.net/websvn/rev ... &peg=32980
At least the part with Evolve triggers and stuff. They are now being processed internally together with the IDs of cards they come from, so two evolve triggers from different creatures should not automatically order as if they were the same.
Granted, this change does not help with the Amulet of Vigor problem. Can you please single out your specific proposal for that part of the issue and provide the code that you believe would fix it?
Thanks in advance for your input!- Agetian
- Agetian
- Programmer
- Posts: 3471
- Joined: 14 Mar 2011, 05:58
- Has thanked: 676 times
- Been thanked: 561 times
Re: Bug Reports (snapshot builds)
by pfps » 06 Jan 2017, 02:21
Hi:
Thanks for the quick reply.
For starters, here is the desktop GUI change to allow viewing cards for triggers (wrapped abilities). It doesn't fix anything but does make viewing what is going on easier.
===================================================================
--- forge-gui-desktop/src/main/java/forge/gui/DualListBox.java (revision 32943)
+++ forge-gui-desktop/src/main/java/forge/gui/DualListBox.java (working copy)
@@ -25,6 +25,7 @@
import forge.game.card.CardView;
import forge.game.card.CardView.CardStateView;
import forge.game.spellability.SpellAbilityView;
+import forge.game.trigger.WrappedAbility;
import forge.item.IPaperCard;
import forge.item.PaperCard;
import forge.screens.match.CMatchUI;
@@ -336,6 +337,8 @@
card = ((SpellAbilityView) obj).getHostCard();
} else if (obj instanceof PaperCard) {
card = Card.getCardForUi((IPaperCard) obj).getView();
+ } else if (obj instanceof WrappedAbility) {
+ card = ((WrappedAbility) obj).getTrigger().getCardView();
} else {
card = null;
}
Thanks for the quick reply.
For starters, here is the desktop GUI change to allow viewing cards for triggers (wrapped abilities). It doesn't fix anything but does make viewing what is going on easier.
===================================================================
--- forge-gui-desktop/src/main/java/forge/gui/DualListBox.java (revision 32943)
+++ forge-gui-desktop/src/main/java/forge/gui/DualListBox.java (working copy)
@@ -25,6 +25,7 @@
import forge.game.card.CardView;
import forge.game.card.CardView.CardStateView;
import forge.game.spellability.SpellAbilityView;
+import forge.game.trigger.WrappedAbility;
import forge.item.IPaperCard;
import forge.item.PaperCard;
import forge.screens.match.CMatchUI;
@@ -336,6 +337,8 @@
card = ((SpellAbilityView) obj).getHostCard();
} else if (obj instanceof PaperCard) {
card = Card.getCardForUi((IPaperCard) obj).getView();
+ } else if (obj instanceof WrappedAbility) {
+ card = ((WrappedAbility) obj).getTrigger().getCardView();
} else {
card = null;
}
Re: Bug Reports (snapshot builds)
by pfps » 06 Jan 2017, 02:58
OK, I'm looking at orderAndPlaySimultaneousSa in r32985 (to figure out why it actually works). It seems to me that the logic isn't right. The card ID isn't added to firstStr, so the compare will always compare different.
My solution is to use the stack description here and elsewhere. This works for Amulet of Vigor because the stack description of wrapped abilities has some stuff at the end that shows the important triggering stuff, i.e., the card that entered tapped. This not only fixes the problem with different triggers being considered equivalent but also provides the information to the user in the ordering box (admittedly usually far off to the right. I'll provide an annotated diff in the next message.
My solution is to use the stack description here and elsewhere. This works for Amulet of Vigor because the stack description of wrapped abilities has some stuff at the end that shows the important triggering stuff, i.e., the card that entered tapped. This not only fixes the problem with different triggers being considered equivalent but also provides the information to the user in the ordering box (admittedly usually far off to the right. I'll provide an annotated diff in the next message.
Re: Bug Reports (snapshot builds)
by pfps » 06 Jan 2017, 03:31
OK here are the changes I made and an explanation of what is going on. (I
don't know how to make the diffs look nice.)
I changed the string for wrapped abilities (triggers and maybe other stuff).
This makes different abilities look different when considering whether to
ask the user to order simultaneous abilities. This also gives the human
player the information needed to see what an ability actually is doing when
ordering simultaneous abilities.
First, in WrappedAbility.java use the stack description as that now has the
important triggering information at the end. This works for the untap
ability of Amulet of Vigor and most other abilities.
Second, in WrappedAbility.java add in the host card string representation if
that isn't already in the string and the string has " this " in it.
This is for abliities like Evolve that don't have CARDNAME in them but that
affect the host card. (A better solution would be to actually change the
wording for Evolve and all other such triggers to use CARDNAME.)
Third, in Trigger.java add a new variant of toString() for triggers that can
switch between using card names and card toString() and, in
WrappedAbility.java, use it for wrapped abilities. This is to distinguish
between cards with the same name when determining whether two wrapped
abilities are equivalent.
The interesting aspect of this change is that it if there are two different
Amulet of Vigor in play and one permanent comes into play tapped the two
effects will have the same string representation as there is no reason to
add the identifier of the Amulet of Vigor to the string representation.
There are still cases where the string representations are different but the
effects will be the same, such as two creatures coming into play
simultaneously and both triggering Evolve on the same creature.
This is done in a bit sneaky way beacuse toString() is final in a parent
and can't be changed here. It might be better to make toString() not final
in the parent and override it directly in WrappedAbility.
Index: forge-game/src/main/java/forge/game/trigger/WrappedAbility.java
===================================================================
--- forge-game/src/main/java/forge/game/trigger/WrappedAbility.java (revision 32943)
+++ forge-game/src/main/java/forge/game/trigger/WrappedAbility.java (working copy)
@@ -191,12 +191,17 @@
@Override
public String toUnsuppressedString() {
- return regtrig.toString();
+ String strg = this.getStackDescription(); /* use augmented stack description as string for wrapped things */
+ String card = regtrig.getHostCard().toString();
+ if ( !strg.contains(card) && strg.contains(" this ")) { /* a hack for Evolve and similar that don't have CARDNAME */
+ return card + ": " + strg;
+ } else return strg;
+ /* return regtrig.toString(); */
}
@Override
public String getStackDescription() {
- final StringBuilder sb = new StringBuilder(regtrig.replaceAbilityText(toUnsuppressedString(), this));
+ final StringBuilder sb = new StringBuilder(regtrig.replaceAbilityText(regtrig.toString(true), this));
if (usesTargeting()) {
sb.append(" (Targeting ");
for (final GameObject o : this.getTargets().getTargets()) {
Index: forge-game/src/main/java/forge/game/trigger/Trigger.java
==================================================================
--- forge-game/src/main/java/forge/game/trigger/Trigger.java (revision 32943)
+++ forge-game/src/main/java/forge/game/trigger/Trigger.java (working copy)
@@ -146,13 +146,17 @@
*/
@Override
public final String toString() {
+ return toString(false);
+ }
+
+ public String toString(boolean active) {
if (this.mapParams.containsKey("TriggerDescription") && !this.isSuppressed()) {
StringBuilder sb = new StringBuilder();
String desc = this.mapParams.get("TriggerDescription");
- desc = desc.replace("CARDNAME", getHostCard().getName());
+ desc = desc.replace("CARDNAME", active ? getHostCard().toString() : getHostCard().getName());
if (getHostCard().getEffectSource() != null) {
- desc = desc.replace("EFFECTSOURCE", getHostCard().getEffectSource().getName());
+ desc = desc.replace("EFFECTSOURCE", active ? getHostCard().getEffectSource().toString() : getHostCard().getEffectSource().getName());
}
sb.append(desc);
if (!this.triggerRemembered.isEmpty()) {
don't know how to make the diffs look nice.)
I changed the string for wrapped abilities (triggers and maybe other stuff).
This makes different abilities look different when considering whether to
ask the user to order simultaneous abilities. This also gives the human
player the information needed to see what an ability actually is doing when
ordering simultaneous abilities.
First, in WrappedAbility.java use the stack description as that now has the
important triggering information at the end. This works for the untap
ability of Amulet of Vigor and most other abilities.
Second, in WrappedAbility.java add in the host card string representation if
that isn't already in the string and the string has " this " in it.
This is for abliities like Evolve that don't have CARDNAME in them but that
affect the host card. (A better solution would be to actually change the
wording for Evolve and all other such triggers to use CARDNAME.)
Third, in Trigger.java add a new variant of toString() for triggers that can
switch between using card names and card toString() and, in
WrappedAbility.java, use it for wrapped abilities. This is to distinguish
between cards with the same name when determining whether two wrapped
abilities are equivalent.
The interesting aspect of this change is that it if there are two different
Amulet of Vigor in play and one permanent comes into play tapped the two
effects will have the same string representation as there is no reason to
add the identifier of the Amulet of Vigor to the string representation.
There are still cases where the string representations are different but the
effects will be the same, such as two creatures coming into play
simultaneously and both triggering Evolve on the same creature.
This is done in a bit sneaky way beacuse toString() is final in a parent
and can't be changed here. It might be better to make toString() not final
in the parent and override it directly in WrappedAbility.
Index: forge-game/src/main/java/forge/game/trigger/WrappedAbility.java
===================================================================
--- forge-game/src/main/java/forge/game/trigger/WrappedAbility.java (revision 32943)
+++ forge-game/src/main/java/forge/game/trigger/WrappedAbility.java (working copy)
@@ -191,12 +191,17 @@
@Override
public String toUnsuppressedString() {
- return regtrig.toString();
+ String strg = this.getStackDescription(); /* use augmented stack description as string for wrapped things */
+ String card = regtrig.getHostCard().toString();
+ if ( !strg.contains(card) && strg.contains(" this ")) { /* a hack for Evolve and similar that don't have CARDNAME */
+ return card + ": " + strg;
+ } else return strg;
+ /* return regtrig.toString(); */
}
@Override
public String getStackDescription() {
- final StringBuilder sb = new StringBuilder(regtrig.replaceAbilityText(toUnsuppressedString(), this));
+ final StringBuilder sb = new StringBuilder(regtrig.replaceAbilityText(regtrig.toString(true), this));
if (usesTargeting()) {
sb.append(" (Targeting ");
for (final GameObject o : this.getTargets().getTargets()) {
Index: forge-game/src/main/java/forge/game/trigger/Trigger.java
==================================================================
--- forge-game/src/main/java/forge/game/trigger/Trigger.java (revision 32943)
+++ forge-game/src/main/java/forge/game/trigger/Trigger.java (working copy)
@@ -146,13 +146,17 @@
*/
@Override
public final String toString() {
+ return toString(false);
+ }
+
+ public String toString(boolean active) {
if (this.mapParams.containsKey("TriggerDescription") && !this.isSuppressed()) {
StringBuilder sb = new StringBuilder();
String desc = this.mapParams.get("TriggerDescription");
- desc = desc.replace("CARDNAME", getHostCard().getName());
+ desc = desc.replace("CARDNAME", active ? getHostCard().toString() : getHostCard().getName());
if (getHostCard().getEffectSource() != null) {
- desc = desc.replace("EFFECTSOURCE", getHostCard().getEffectSource().getName());
+ desc = desc.replace("EFFECTSOURCE", active ? getHostCard().getEffectSource().toString() : getHostCard().getEffectSource().getName());
}
sb.append(desc);
if (!this.triggerRemembered.isEmpty()) {
Re: Bug Reports (snapshot builds)
by friarsol » 06 Jan 2017, 03:36
Pfps, it looks like you were added to the forge developers the last time you were trying to improve somethings.. this looks fine to me if you want to commit yourself
The best way to make the diff blocks look better is to wrap them in the code entity in the editor
The best way to make the diff blocks look better is to wrap them in the code entity in the editor
- friarsol
- Global Moderator
- Posts: 7593
- Joined: 15 May 2010, 04:20
- Has thanked: 243 times
- Been thanked: 965 times
Re: Bug Reports (snapshot builds)
by Agetian » 06 Jan 2017, 04:46
@ pfps: Ah, thanks a lot for pointing out an issue with my commit, you're right! And I like your idea too, I'll see if I can find a minute to apply it and commit as a part of fixing the faulty logic that you identified in orderAndPlaySimultaneousSa, but if I don't manage to do it before you read the message, you're definitely very welcome to commit it yourself too! Thanks for your contributions!
EDIT: Committed your changes and reverted my change to orderAndPlaySimultaneousSa.
- Agetian
And I like your idea too, I'll see if I can find a minute to apply it and commit as a part of fixing the faulty logic that you identified in orderAndPlaySimultaneousSa, but if I don't manage to do it before you read the message, you're definitely very welcome to commit it yourself too! Thanks for your contributions! EDIT: Committed your changes and reverted my change to orderAndPlaySimultaneousSa.
- Agetian
- Agetian
- Programmer
- Posts: 3471
- Joined: 14 Mar 2011, 05:58
- Has thanked: 676 times
- Been thanked: 561 times
- Agetian
- Programmer
- Posts: 3471
- Joined: 14 Mar 2011, 05:58
- Has thanked: 676 times
- Been thanked: 561 times
Re: Bug Reports (snapshot builds)
by pfps » 06 Jan 2017, 15:38
OK, I'm running r32993 and things are mostly working.
I am seeing something strange with repeated ordering. I had 2 Amulets out and untapped using them first and then bounced a bounce land back. The next time I was offered to untap, then bounce, then untap, which wasn't what I had just done. I'll investigate further.
It also occurs to me that the ordering confirmation box isn't necessary. Just make that the initial order instead (and say something in the dual list box title, I guess). I'll see if I can make this work.
I am seeing something strange with repeated ordering. I had 2 Amulets out and untapped using them first and then bounced a bounce land back. The next time I was offered to untap, then bounce, then untap, which wasn't what I had just done. I'll investigate further.
It also occurs to me that the ordering confirmation box isn't necessary. Just make that the initial order instead (and say something in the dual list box title, I guess). I'll see if I can make this work.
Re: Bug Reports (snapshot builds)
by Agetian » 06 Jan 2017, 15:49
Well, the ordering confirmation was implemented to be able to change the order if necessary, that was discussed before and there were cases where this was relevant. It would be nice to still be able to change the order.pfps wrote:OK, I'm running r32993 and things are mostly working.
I am seeing something strange with repeated ordering. I had 2 Amulets out and untapped using them first and then bounced a bounce land back. The next time I was offered to untap, then bounce, then untap, which wasn't what I had just done. I'll investigate further.
It also occurs to me that the ordering confirmation box isn't necessary. Just make that the initial order instead (and say something in the dual list box title, I guess). I'll see if I can make this work.
- Agetian
- Agetian
- Programmer
- Posts: 3471
- Joined: 14 Mar 2011, 05:58
- Has thanked: 676 times
- Been thanked: 561 times
Re: Bug Reports (snapshot builds)
by fmartel » 06 Jan 2017, 16:10
-r32997, in commander, the "Auto yeild" does not work for Soul Warden. I always have to push "Entre" to confirm the action. Even If I right-click and tell it to "Atomate" it.
- fmartel
- Posts: 281
- Joined: 31 Dec 2013, 19:27
- Location: Québec City
- Has thanked: 8 times
- Been thanked: 4 times
Re: Bug Reports (snapshot builds)
by friarsol » 06 Jan 2017, 16:21
This is definitely related to the ordering change. Since the triggers no longer "match" it isn't passed for auto-trigger handling.fmartel wrote:-r32997, in commander, the "Auto yeild" does not work for Soul Warden. I always have to push "Entre" to confirm the action. Even If I right-click and tell it to "Atomate" it.
- friarsol
- Global Moderator
- Posts: 7593
- Joined: 15 May 2010, 04:20
- Has thanked: 243 times
- Been thanked: 965 times
Re: Bug Reports (snapshot builds)
by fmartel » 06 Jan 2017, 16:29
Description: [in commander, choosing a land with Sword of the Animist]
- OutOfMemoryError | Open
- fmartel
- Posts: 281
- Joined: 31 Dec 2013, 19:27
- Location: Québec City
- Has thanked: 8 times
- Been thanked: 4 times
Re: Bug Reports (snapshot builds)
by Agetian » 06 Jan 2017, 17:01
Hmm, I think something needs to be done about this, otherwise auto yielding will become useless... should we temporarily revert until this can be properly thought through?friarsol wrote:This is definitely related to the ordering change. Since the triggers no longer "match" it isn't passed for auto-trigger handling.fmartel wrote:-r32997, in commander, the "Auto yeild" does not work for Soul Warden. I always have to push "Entre" to confirm the action. Even If I right-click and tell it to "Atomate" it.
EDIT: Temporarily reverted until this can be addressed, feel free to revert my revert to continue working on this project (and also commit the updated version)
- Agetian
Last edited by Agetian on 06 Jan 2017, 19:00, edited 1 time in total.
- Agetian
- Programmer
- Posts: 3471
- Joined: 14 Mar 2011, 05:58
- Has thanked: 676 times
- Been thanked: 561 times
Re: Bug Reports (snapshot builds)
by timmermac » 06 Jan 2017, 17:46
The amount of physical RAM your machine has isn't relevant here. The key is how much of that is allocated for use by Java. If you right-click on forge.exe and click properties, you should see a window that, among other things, has a box that lists some parameters. Where it says Xms, you'll want to modify the number there to 4096. That means that Java now has access to 4 gigs of memory. I'm not sure if Java will let you allocate more or not.fmartel wrote:Description: [in commander, choosing a land with Sword of the Animist]Note that I have 24GB RAM machine
- OutOfMemoryError | Open
"I just woke up, haven't had coffee, let alone a pee in 7 days, and I find out you stole my ass and made a ...mini-me! Carter, I should be irked currently, yes?" - Jack O'Neill
Who is online
Users browsing this forum: No registered users and 46 guests