Hard Light Productions Forums

Modding, Mission Design, and Coding => FS2 Open Coding - The Source Code Project (SCP) => Topic started by: zookeeper on February 09, 2014, 12:58:21 pm

Title: Mystery bug
Post by: zookeeper on February 09, 2014, 12:58:21 pm

I set off to allow the $Weapon class condition to be used with the $On Weapon Collision scripting hook, which ought to be simple enough:

Code: [Select]

Index: object/collidedebrisweapon.cpp
===================================================================
--- object/collidedebrisweapon.cpp (revision 10393)
+++ object/collidedebrisweapon.cpp (working copy)
@@ -72,7 +72,7 @@
 
  Script_system.SetHookObjects(2, "Self",pdebris, "Object", weapon);
  if((debris_override && !weapon_override) || (!debris_override && !weapon_override))
- Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, pdebris);
+ Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, pdebris, Weapons[weapon->instance].weapon_info_index);
 
  Script_system.RemHookVars(4, "Weapon", "Debris", "Self","ObjectB");
  return 0;
Index: object/collideshipweapon.cpp
===================================================================
--- object/collideshipweapon.cpp (revision 10420)
+++ object/collideshipweapon.cpp (working copy)
@@ -421,7 +421,7 @@
 
  Script_system.SetHookObjects(2, "Self",ship_objp, "Object", weapon_objp);
  if(!(weapon_override && !ship_override))
- Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, ship_objp);
+ Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, ship_objp, wp->weapon_info_index);
 
  Script_system.SetHookObjects(2, "Self",weapon_objp, "Object", ship_objp);
  if((weapon_override && !ship_override) || (!weapon_override && !ship_override))
Index: object/collideweaponweapon.cpp
===================================================================
--- object/collideweaponweapon.cpp (revision 10394)
+++ object/collideweaponweapon.cpp (working copy)
@@ -179,13 +179,13 @@
  if(!(b_override && !a_override))
  {
  Script_system.SetHookObjects(4, "Weapon", A, "WeaponB", B, "Self",A, "Object", B);
- Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, A);
+ Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, A, wpA->weapon_info_index);
  }
  if((b_override && !a_override) || (!b_override && !a_override))
  {
  //Should be reversed
  Script_system.SetHookObjects(4, "Weapon", B, "WeaponB", A, "Self",B, "Object", A);
- Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, B);
+ Script_system.RunCondition(CHA_COLLIDEWEAPON, '\0', NULL, B, wpB->weapon_info_index);
  }
 
  Script_system.RemHookVars(4, "Weapon", "WeaponB", "Self","ObjectB");
Index: parse/scripting.cpp
===================================================================
--- parse/scripting.cpp (revision 10393)
+++ parse/scripting.cpp (working copy)
@@ -311,6 +311,9 @@
  }
  case CHC_WEAPONCLASS:
  {
+ if(action == CHA_COLLIDEWEAPON && stricmp(Weapon_info[more_data].name, scp->data.name) != 0) {
+ return false;
+ } else
  if (!(action == CHA_ONWPSELECTED || action == CHA_ONWPDESELECTED || action == CHA_ONWPEQUIPPED || action == CHA_ONWPFIRED || action == CHA_ONTURRETFIRED )) {
  if(objp == NULL || (objp->type != OBJ_WEAPON && objp->type != OBJ_BEAM))
  return false;
...and a test script to go with it (shoot something with a subach to trigger the message, on a debug build):

Code: [Select]

#Conditional Hooks

$Weapon class: Subach HL-7

$On Weapon Collision: [

    ba.warning("triggered")

]

#End

Now, look at that last code block I added, in scripting.cpp:

Code: [Select]

if(action == CHA_COLLIDEWEAPON && stricmp(Weapon_info[more_data].name, scp->data.name) != 0) {
return false;
}
It doesn't work. However -and this is where it gets weird- if you change that to for example this, then it does work:

Code: [Select]

if(action == CHA_COLLIDEWEAPON) {
    if (stricmp(Weapon_info[more_data].name, scp->data.name) != 0) {
        return false;
    }
}
If that's not weird enough, then even this works:

Code: [Select]

if(action == CHA_COLLIDEWEAPON) {
    if ((action == CHA_COLLIDEWEAPON) && stricmp(Weapon_info[more_data].name, scp->data.name) != 0) {
        return false;
    }
}
It doesn't make any sense. Compiler bug? Some I-don't-have-a-word-for-it memory bug? Can anyone even reproduce this?

So, to clarify how to reproduce: apply the patch, and the test script will not work. Change the if statement to for example either of the given alternatives, and the script should start working.

Title: Re: Mystery bug
Post by: Echelon9 on February 09, 2014, 08:55:59 pm

Sounds like an order of precedence bug. This manifests in undefined or unexpected behaviour when the evaluation order of a multi-part if() statement is not make explicit.

c.f. http://stackoverflow.com/questions/7925479/if-argument-evaluation-order

Without making order explicit, each compiler may produce slightly different if() branching, including at different optimisation levels for the same compiler.

I haven't tested this, but I would presume the following would work as expected:

Code: [Select]

if ((action == CHA_COLLIDEWEAPON) && (stricmp(Weapon_info[more_data].name, scp->data.name) != 0)) {
  return false;
 }

Title: Re: Mystery bug
Post by: zookeeper on February 10, 2014, 02:18:20 am

Nope, I've tried that, no difference.

Title: Re: Mystery bug
Post by: m!m on February 10, 2014, 03:38:21 am

Can VS open a disassembly of a specific function? Looking at that could help figure out what the compiler is actually doing.

Title: Re: Mystery bug
Post by: niffiwan on February 10, 2014, 04:44:01 am

In the non-working case, if the 1st if statement evaluates to false, won't it check the rest of the else if statements?  And if one of those also match, it could/will return false at that point?

Whereas in the working case, if action == CHA_COLLIDEWEAPON is true, but (stricmp(Weapon_info[more_data].name, scp->data.name) != 0) is not then the rest of the if-else conditions will be skipped?

Title: Re: Mystery bug
Post by: zookeeper on February 10, 2014, 04:59:47 am

Quote from: niffiwan on February 10, 2014, 04:44:01 am

In the non-working case, if the 1st if statement evaluates to false, won't it check the rest of the else if statements?  And if one of those also match, it could/will return false at that point?

Whereas in the working case, if action == CHA_COLLIDEWEAPON is true, but (stricmp(Weapon_info[more_data].name, scp->data.name) != 0) is not then the rest of the if-else conditions will be skipped?

:eek2: Yes, that makes sense! That's exactly what would happen. Thanks. :D

Title: Re: Mystery bug
Post by: Aardwolf on February 10, 2014, 02:39:12 pm

So there was an else omitted in the post, and that's where the bug was?

Title: Re: Mystery bug
Post by: niffiwan on February 10, 2014, 03:54:40 pm

The diff showed the 1st two statements in a bunch of if-else if-else statements, it was looking at the rest of those that gave me the idea about what was going on - the bug was still in the posted code though.

Code: (http://svn.icculus.org/*checkout*/fs2open/trunk/fs2_open/code/parse/scripting.cpp?revision=10430&content-type=text%2Fplain&pathrev=10430) [Select]

if (action == CHA_COLLIDEWEAPON) {
if (stricmp(Weapon_info[more_data].name, scp->data.name) != 0)
return false;
} else if (!(action == CHA_ONWPSELECTED || action == CHA_ONWPDESELECTED || action == CHA_ONWPEQUIPPED || action == CHA_ONWPFIRED || action == CHA_ONTURRETFIRED )) {
if(objp == NULL || (objp->type != OBJ_WEAPON && objp->type != OBJ_BEAM))
return false;
else if (( objp->type == OBJ_WEAPON) && (stricmp(Weapon_info[Weapons[objp->instance].weapon_info_index].name, scp->data.name) != 0 ))
return false;
else if (( objp->type == OBJ_BEAM) && (stricmp(Weapon_info[Beams[objp->instance].weapon_info_index].name, scp->data.name) != 0 ))
return false;
} else if(objp == NULL || objp->type != OBJ_SHIP) {
return false;
} else {
...


Title: Re: Mystery bug
Post by: Goober5000 on February 17, 2014, 02:46:17 pm

Closed as this seems to be resolved.  As a reminder, bug reports belong on Mantis. ;)