[confirmed]Coffin Queen dumps
Report wrong Card behavior to get it fixed.
PLEASE ADD SAVEGAMES TO YOUR TOPIC !
PLEASE ADD SAVEGAMES TO YOUR TOPIC !
Moderators: BAgate, drool66, Aswan jaguar, gmzombie, stassy, CCGHQ Admins
[confirmed]Coffin Queen dumps
by FastEddie » 01 Nov 2020, 13:12
Describe the Bug:
Coffin Queen dumps when activated. I looked into it and found a few interesting things, but didn't get any further.
The dump itself is caused by charge_mana() which is called by charge_mana_multi() to charge the activation cost. Up to that point all parameters look good, i.e. generic_activated_ability_with_target_in_grave and everything coming afterwards uses the correct card id (that of Coffin Queen) and the correct player.
The first paramter of in_play in the dump is the index of the card in the graveyard, 0 for the first card, 1 for the second, etc. while this should be the id of the player (0 or 1). So somewhere between calling charge_mana() and calling in_play() things get mixed up.
There is one more issue which I (obviously) couldn't test. I think the code inside Coffin Queen's EVENT_RESOLVE_ACTIVATION should rather look like this:
Which card behaved improperly?
Coffin Queen
Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
Version 10-2020 (bug existed also under version 08-2020)
Duel
What exactly should be the correct behavior/interaction?
Coffin Queen should not dump and return the proper creature.
Are any other cards possibly affected by this bug?
Depends on what the issue is. There could be other cards using a similar graveyard machanic.
Coffin Queen dumps when activated. I looked into it and found a few interesting things, but didn't get any further.
The dump itself is caused by charge_mana() which is called by charge_mana_multi() to charge the activation cost. Up to that point all parameters look good, i.e. generic_activated_ability_with_target_in_grave and everything coming afterwards uses the correct card id (that of Coffin Queen) and the correct player.
The first paramter of in_play in the dump is the index of the card in the graveyard, 0 for the first card, 1 for the second, etc. while this should be the id of the player (0 or 1). So somewhere between calling charge_mana() and calling in_play() things get mixed up.
There is one more issue which I (obviously) couldn't test. I think the code inside Coffin Queen's EVENT_RESOLVE_ACTIVATION should rather look like this:
- Code: Select all
if (selected != -1){
// generic_activated_ability_with_target_in_grave returns graveyard in targets[0] and card in targets[1]
int zombie = reanimate_permanent(player, card, instance->targets[0].player, selected, REANIMATE_DEFAULT | REANIMATE_NO_CONTROL_LEGACY);
if( zombie > -1 ){
- | Open
- bad parameters
in_play(4, 234886677)
0: 0x02550738
1: 0x0250315B
2: 0x0251E44C
3: 0x0216DEE5
4: 0x02560C55
5: 0x0253C0CD
6: 0x0253CC0E
7: 0x00435A13
8: 0x02510482
9: 0x00472273
10: 0x004379C4
11: 0x0049CBFA
12: 0x0250F4F9
13: 0x02526AA6
14: 0x02533534
15: 0x025393C9
16: 0x0216DF98
17: 0x02560C55
18: 0x0253C0CD
19: 0x0253C7F6
20: 0x004344B2
21: 0x0043C147
22: 0x004399BD
23: 0x0047902C
24: 0x004946E9
25: 0x77A56359
26: 0x77E27C24
27: 0x77E27BF4
28: 0x00000000
Which card behaved improperly?
Coffin Queen
Which update are you using? (date, name)Which type? (duel, gauntlet, sealed deck)
Version 10-2020 (bug existed also under version 08-2020)
Duel
What exactly should be the correct behavior/interaction?
Coffin Queen should not dump and return the proper creature.
Are any other cards possibly affected by this bug?
Depends on what the issue is. There could be other cards using a similar graveyard machanic.
- Attachments
-
- CoffinQueen_dump.zip
- (410 Bytes) Downloaded 103 times
-
- CoffinQueen.zip
- (3.3 KiB) Downloaded 112 times
Last edited by Aswan jaguar on 01 Nov 2020, 14:10, edited 1 time in total.
Reason: confirmed
Reason: confirmed
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by Aswan jaguar » 04 Nov 2020, 07:24
There seems to be some code of choose_to_untap() that conflicts with GS_GRAVE_RECYCLER_BOTH_GRAVES mode in generic_activated_ability_with_target_in_grave(). If you disable either of those it will not error and crash and you will get to see the mana prompt. If you disable choose_to_untap() and if you fix the bug in reanimate_permanent() as said in post above you will see another bug that the animated creature will not be exiled if Coffin Queen untaps or leaves play.
The EMN version of Coffin Queen worked fine. If we don't manage to fix it using current method I will see if that still works.
The EMN version of Coffin Queen worked fine. If we don't manage to fix it using current method I will see if that still works.
---
Trying to squash some bugs and playtesting.
Trying to squash some bugs and playtesting.
-
Aswan jaguar - Super Tester Elite
- Posts: 8074
- Joined: 13 May 2010, 12:17
- Has thanked: 729 times
- Been thanked: 455 times
Re: [confirmed]Coffin Queen dumps
by drool66 » 04 Nov 2020, 08:59
Looks like it. generic_activated_ability_with_target_in_grave() with GS_GRAVE_RECYCLER_BOTH_GRAVES uses select_target_from_either_grave() with ret_location == 1; choose_to_untap() continuously checks in_play(targets[1].player, targets[1].card) - hence the bad instance. I can't grok how exactly choose_to_untap() works, especially what the targets[1] marker is for. Maybe someone else can take a look?There seems to be some code of choose_to_untap() that conflicts with GS_GRAVE_RECYCLER_BOTH_GRAVES
The latest images for Manalink will be here.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
-
drool66 - Programmer
- Posts: 1163
- Joined: 25 Nov 2010, 22:38
- Has thanked: 186 times
- Been thanked: 267 times
Re: [confirmed]Coffin Queen dumps
by FastEddie » 04 Nov 2020, 10:12
Thanks guys. I will have another look later this week.
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by drool66 » 04 Nov 2020, 20:24
Well, I looked at this when I was pretty tired last night, and the targets[1] thing is right there in the comments. The choose_to_untap() function could use some work. The way it works now, if targets[1] is not in play, the AI will untap the permanent. One problem in addition to the one at hand is that few of the cards that use this function actually load their target to targets[1]. That's why the AI's Old Man of the Sea will release its target every turn. Additionally, targets[1] just seems like a very unsafe place to store this sort of thing, as we see with Coffin Queen. FastEddie, if you want to take this on, maybe think about storing it in an attached legacy? Lmk if you would rather I take care of it.
I think you mentioned you don't have a working grep, so here are the hits for choose_to_untap():
alliances:3772
antiquities:377, 1594, 2359, 2428
arabian_nights: 2102
fallen_empires:975, 3491
fifth_dawn:4115
homelands:1487
ice_age:3888, 5436
journey_into_nyx:4129
legends:6879, 9005
mercadian_masques:3836
mirage:671, 2897
scars_of_mirrodin:5448
tempest:1020, 2668
the_dark:2486
urza_legacy:2187
I think you mentioned you don't have a working grep, so here are the hits for choose_to_untap():
alliances:3772
antiquities:377, 1594, 2359, 2428
arabian_nights: 2102
fallen_empires:975, 3491
fifth_dawn:4115
homelands:1487
ice_age:3888, 5436
journey_into_nyx:4129
legends:6879, 9005
mercadian_masques:3836
mirage:671, 2897
scars_of_mirrodin:5448
tempest:1020, 2668
the_dark:2486
urza_legacy:2187
The latest images for Manalink will be here.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
-
drool66 - Programmer
- Posts: 1163
- Joined: 25 Nov 2010, 22:38
- Has thanked: 186 times
- Been thanked: 267 times
Re: [confirmed]Coffin Queen dumps
by FastEddie » 05 Nov 2020, 07:18
I will take care of that. My suspicion was that this is water on Korath's mills, namely that storing data inside cards is usually a bad idea (but not always). My idea was indeed a targeted legacy and I will add a find function to make searching for those easier.
I saw something similar at Leonin Arbiter and added a find function for legacy effects, but this isn't committed yet. With create and find functions storing data in legacies should be as simple as storing data in cards and way safer to boot.
I saw something similar at Leonin Arbiter and added a find function for legacy effects, but this isn't committed yet. With create and find functions storing data in legacies should be as simple as storing data in cards and way safer to boot.
Last edited by FastEddie on 05 Nov 2020, 07:19, edited 1 time in total.
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by FastEddie » 05 Nov 2020, 07:19
Re grep, I never bothered as find in files in the IDE does the job for me, but thanks for the effort!
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by Aswan jaguar » 05 Nov 2020, 13:55
drool66 you have committed some changes to choose_to_untap() are those a temporary fix, until it gets fixed with a legacy?
---
Trying to squash some bugs and playtesting.
Trying to squash some bugs and playtesting.
-
Aswan jaguar - Super Tester Elite
- Posts: 8074
- Joined: 13 May 2010, 12:17
- Has thanked: 729 times
- Been thanked: 455 times
Re: [confirmed]Coffin Queen dumps
by drool66 » 05 Nov 2020, 15:35
No, I committed them by mistake because I was working on three or four things at once. I don't think they do anything and they definitely don't belong in that commit. They'll almost definitely be overwritten once this bug is squashed though.
The latest images for Manalink will be here.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
-
drool66 - Programmer
- Posts: 1163
- Joined: 25 Nov 2010, 22:38
- Has thanked: 186 times
- Been thanked: 267 times
Re: [confirmed]Coffin Queen dumps
by FastEddie » 07 Nov 2020, 10:24
The lady made some progress but in return I got weird side effects.
Coffin Queen looks like this now:
Any ideas what could cause this?
Coffin Queen looks like this now:
- Code: Select all
int card_coffin_queen(int player, int card, event_t event){
/* CARD_ID_COFFIN_QUEEN 3106
Coffin Queen |2|B
Creature - Zombie Wizard 1/1
You may choose not to untap ~ during your untap step.
|2|B, |T: Put target creature card from a graveyard onto the battlefield under your control. When ~ becomes untapped or you lose control of ~, exile that creature.*/
// Call this first, otherwise no legacy will be created
choose_to_untap(player, card, event);
if( ! IS_GAA_EVENT(event) ){
return 0;
}
test_definition_t test;
new_default_test_definition(&test, TYPE_CREATURE, "Select target creature card.");
if (event == EVENT_RESOLVE_ACTIVATION){
card_instance_t* instance = get_card_instance(player, card);
int selected = validate_target_from_grave_source(player, card, instance->targets[0].player, 1);
if (selected != -1){
// generic_activated_ability_with_target_in_grave returns graveyard in targets[0] and card in targets[1]
int zombie = reanimate_permanent(player, card, instance->targets[0].player, selected, REANIMATE_DEFAULT | REANIMATE_NO_CONTROL_LEGACY);
if( zombie > -1 ){
int legacy = create_targetted_legacy_effect(instance->parent_controller, instance->parent_card, &cq_effect, player, zombie);
card_instance_t *leg = get_card_instance(player, legacy);
leg->targets[0].player = instance->parent_controller;
leg->targets[0].card = instance->parent_card;
leg->number_of_targets = 1;
// card_instance_t *parent = get_card_instance(instance->parent_controller, instance->parent_card);
// parent->targets[1] = instance->targets[0];
set_choose_to_untap_target(player, card, player, zombie);
}
}
}
return generic_activated_ability_with_target_in_grave(player, card, event, 0, MANACOST_XB(2,1), 0, NULL, NULL,
0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);
}
- Code: Select all
/* Find a targetted legacy effect created by create_targetted_legacy_effect(player, card, func_ptr, target_player, target_card).
* Player, function pointer: same as used for create_legacy_effect.
* Card: id returned by create_legacy_effect.
* Return value: card id if legacy was found, otherwise -1. */
int find_targetted_legacy_effect(int player, int card, int (*func_ptr)(int, int, event_t), int target_player, int target_card){
int result;
card_instance_t* leg_inst;
for ( result=0; result < active_cards_count[player]; result++ ) {
if( (leg_inst = in_play(player, result))
&& leg_inst->internal_card_id == LEGACY_EFFECT_CUSTOM && leg_inst->info_slot == (int)func_ptr
&& leg_inst->damage_source_player == player && leg_inst->damage_source_card == card
&& leg_inst->targets[0].player == target_player && leg_inst->targets[0].card == target_card
){
return result;
}
}
return -1;
}
/* Make untapping card optional. Used for "As long as card ~ is tapped do something to another card" effects.
* Call at the beginning of the card code.
* Choose_to_untap tracks a target card (see also set_choose_to_untap_target). The AI will untap if this card leaves play. */
void choose_to_untap(int player, int card, event_t event){
if( event == EVENT_CAST_SPELL && affect_me(player, card) ){
set_special_flags2(player, card, SF2_COULD_NOT_UNTAP);
}
if ( event == EVENT_RESOLVE_SPELL ) {
// Create a targetted legacy as storage for target card (AI will untap when this card leaves play)
int legacy = create_targetted_legacy_effect(player, card, &empty, player, card);
ASSERT(legacy != -1); // This should never happen but... better safe than sorry and it says more than a generic get_card_instance dump
card_instance_t *legacy_inst = get_card_instance(player, legacy);
legacy_inst->targets[1].player = legacy_inst->targets[1].card = -1; // Should be unnecessary but since we rely on this make it explicit
// TODO: make legacy invisible
return;
}
// Check whether target left play in the meanwhile
int legacy = find_targetted_legacy_effect(player, card, &empty, player, card);
if (legacy != -1) {
// Do not assert here as the legacy may not yet be in play
card_instance_t *legacy_inst = get_card_instance(player, legacy);
if( legacy_inst->targets[1].player > -1 && ! in_play(legacy_inst->targets[1].player, legacy_inst->targets[1].card) ){
legacy_inst->targets[1].player = -1;
}
}
card_instance_t *instance = get_card_instance(player, card);
if (event == EVENT_UNTAP && affect_me(player, card) && is_tapped(player, card)){
instance->untap_status &= ~2;
}
if (current_phase == PHASE_UNTAP && affect_me(player, card)){
if (event == EVENT_TRIGGER){
if (instance->untap_status & 1){ // Card it tapped ...
if (!(instance->untap_status & 2)
&& !(cards_data[instance->internal_card_id].type & types_that_dont_untap)){ // ... and not marked for untapping or that it cannot untap
card_instance_t *legacy_inst = get_card_instance(player, legacy);
if ((player != 1 || (trace_mode & 2)) && ai_is_speculating != 1){
event_result |= 1; // Human: untapping is optional
} else if (legacy_inst->targets[1].player > -1 && in_play(legacy_inst->targets[1].player, legacy_inst->targets[1].card)){
; // AI: leave tapped if target card is still in play
} else {
event_result |= 2; // AI: target card left play, untap
}
}
}
} else if (event == EVENT_RESOLVE_TRIGGER){
instance->untap_status |= 2; // Mark card as "shall untap"
}
}
}
/* Set the target card tracked by choose_to_untap.
* Cards using choose_to_untap must do this manually as there is no other way to know when the target changed.
* player, card: who is calling set_choose_to_untap_target
* target_player, target_card: new target */
void set_choose_to_untap_target(int player, int card, int target_player, int target_card) {
int legacy = find_targetted_legacy_effect(player, card, &empty, player, card);
if ( legacy != -1 ) {
// Do nothing if no legacy exists as this function might have been called from anywhere.
card_instance_t *legacy_inst = get_card_instance(player, legacy);
legacy_inst->targets[1].player = target_player;
legacy_inst->targets[1].card = target_card;
}
return;
}
- Code: Select all
int find_targetted_legacy_effect(int player, int card, int (*func_ptr)(int, int, event_t), int targ_player, int target_card); // Find a legacy effect created by create_targetted_legacy_effect.
void set_choose_to_untap_target(int player, int card, int target_player, int target_card); // Manually set the target tracked by choose_to_untap so that the AI can track it
Any ideas what could cause this?
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by drool66 » 07 Nov 2020, 21:13
I might have some idea - I don't see the code for cq_effect(), can you post that please
[EDIT]
I tried out the code - two changes fix the card:
in cq_effect, you need to change the kill_card to kill instance->damage_target_player/card, that's why coffin queen was dying - she was in that spot instead. She was dying on resolution because you were missing GAA_UNTAPPED in your generic_activated_ability_with_target_in_grave(), and so the !is_tapped() conditional returned true. You skipped your draw phase because the remaining cq_legacy killed the next card with the recycled Coffin Queen player, card designation, which was the card drawn on your draw phase.
You'll see I made "static int find_attached_legacy_effect()" for Muldrotha the Gravetide, which is the same as your "find_targetted_legacy_effect()". If you do commit, would you please tidy that all up for both cards so that we don't have redundant functions?
Otherwise, very impressive work!
[EDIT]
I tried out the code - two changes fix the card:
in cq_effect, you need to change the kill_card to kill instance->damage_target_player/card, that's why coffin queen was dying - she was in that spot instead. She was dying on resolution because you were missing GAA_UNTAPPED in your generic_activated_ability_with_target_in_grave(), and so the !is_tapped() conditional returned true. You skipped your draw phase because the remaining cq_legacy killed the next card with the recycled Coffin Queen player, card designation, which was the card drawn on your draw phase.
You'll see I made "static int find_attached_legacy_effect()" for Muldrotha the Gravetide, which is the same as your "find_targetted_legacy_effect()". If you do commit, would you please tidy that all up for both cards so that we don't have redundant functions?
Otherwise, very impressive work!
The latest images for Manalink will be here.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
The latest Manalink installation directory will be here. Well, not quite, anymore. Check the latest patches.
-
drool66 - Programmer
- Posts: 1163
- Joined: 25 Nov 2010, 22:38
- Has thanked: 186 times
- Been thanked: 267 times
Re: [confirmed]Coffin Queen dumps
by FastEddie » 09 Nov 2020, 12:48
Sure, I will take care of that.You'll see I made "static int find_attached_legacy_effect()" for Muldrotha the Gravetide, which is the same as your "find_targetted_legacy_effect()". If you do commit, would you please tidy that all up for both cards so that we don't have redundant functions?
Making progress one bug at a time... the cq_effect bug was already there but I guess the card never got that far, there was another one in the way.
In the meanwhile the Queen became a one shot lady. You can activate it, everything works, untap, the animated creature is released but then it cannot be activated anymore. GAA_UNTAPPED makes sure it can be activated the first time, but then? If I change it to GAA_NONE when the card is still tapped nothing happens (as the name indicates...) and with GAA_TAPPED the game crashes (after highlighting the card that it can be activated). The last part looks like this now (clearly wrong):
- Code: Select all
// Mark as GAA_UNTAPPED so generic_activated_ability can tap it, otherwise the legacy gets released immediately
generic_activated_ability_flags_t gaa_mode = is_tapped(player, card) ? GAA_NONE : GAA_UNTAPPED;
return generic_activated_ability_with_target_in_grave(player, card, event, gaa_mode, MANACOST_XB(2,1), 0, NULL, NULL,
0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by Aswan jaguar » 09 Nov 2020, 14:42
I don't understand what are you talking about, like drool66 said with the below code for cq_effect():
- Code: Select all
int cq_effect(int player, int card, event_t event){
if( get_card_instance(player, card)->targets[0].player > -1 ){
card_instance_t *instance = get_card_instance(player, card);
int cq_player = instance->targets[0].player;
int cq_card = instance->targets[0].card;
if( ! is_tapped(cq_player, cq_card) || other_leaves_play(player, card, cq_player, cq_card, event) ){
kill_card(instance->damage_target_player, instance->damage_target_card, KILL_EXILE);
}
}
return 0;
}
- Code: Select all
return generic_activated_ability_with_target_in_grave(player, card, event, GAA_UNTAPPED, MANACOST_XB(2,1), 0, NULL, NULL, 0, &test, GS_GRAVE_RECYCLER_BOTH_GRAVES);
---
Trying to squash some bugs and playtesting.
Trying to squash some bugs and playtesting.
-
Aswan jaguar - Super Tester Elite
- Posts: 8074
- Joined: 13 May 2010, 12:17
- Has thanked: 729 times
- Been thanked: 455 times
Re: [confirmed]Coffin Queen dumps
by FastEddie » 09 Nov 2020, 14:54
See, that I didn't know. In the branches I am tracking, master and origin/manalink/master_sep2020 I can see no change.The rest of the code that you have posted and with GAA_UNTAPPED in generic_activated_ability_with_target_in_grave() that I have already fixed upstream days ago
When choose_to_untap has been changed the other cards using it must also be fixed and last time when I tested the AI still untapped Phyrexian Gremlins every turn, so there might be still another issue.
---
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Argivian Archaeologist in the Library of Leng studying the Spells of the Ancients
Re: [confirmed]Coffin Queen dumps
by Aswan jaguar » 09 Nov 2020, 15:49
The important part of my comment above was more to emphasize that Coffin Queen works fine while you said you still got issues. Is it now fine for you, too?
The only think to improve is that the legacy attached on Coffin Queen (or any creature using this function) should be invisible.
The only think to improve is that the legacy attached on Coffin Queen (or any creature using this function) should be invisible.
---
Trying to squash some bugs and playtesting.
Trying to squash some bugs and playtesting.
-
Aswan jaguar - Super Tester Elite
- Posts: 8074
- Joined: 13 May 2010, 12:17
- Has thanked: 729 times
- Been thanked: 455 times
18 posts
• Page 1 of 2 • 1, 2
Who is online
Users browsing this forum: No registered users and 12 guests