Beam autotracking code

[royfalk]

Call me crazy, but I don't think this code does what I think it does.

Unit *autotarg = (isAutoTrackingMount(mounts[i].size)
                                && (mounts[i].time_to_lock <= 0)
                                && TargetTracked()) ? target : NULL;
float tracking_cone = radar.tracking_cone;
if (CloseEnoughToAutotrack(this, target, tracking_cone)) {
   if (autotarg) {
       if (radar.tracking_cone < tracking_cone) {
           tracking_cone = radar.tracking_cone;
        }
   }
   autotarg = target;
}

Simplified a bit and it becomes:

Unit *a = <some condition> ? target : nullptr;
float b = c;
if(some other condition) {
   if(some other condition) {
      b = c; // again!
   }
   a = target; // again!
}

EDIT: (BenjamenMeyer) Fixed formatting

[BenjamenMeyer]

First conversion:

// convert the  ternary ( x = a ? b) into something easier to read
Unit *autotarg = nullptr;  // default to nullptr
if ( 
    isAutoTrackingMount(mounts[i].size) &&
    (mounts[i].time_to_lock <= 0) &&
    TargetTracked()
) {
    // if all the things then set auto target to the target
    autotarg = target;
}

float tracking_cone = radar.tracking_cone;
if (
    CloseEnoughToAutotrack(this, target, tracking_cone)
) { 
   if (autotarg) {
       if (radar.tracking_cone < tracking_cone) {
           tracking_cone = radar.tracking_cone; 
        }  
   }    
   autotarg = target;
}

[BenjamenMeyer]

yeah...there's something fishy...this is how I read it (without further context):

Unit *autotarg = nullptr;
if ( 
    isAutoTrackingMount(mounts[i].size) &&  // if a mount is tracking it
    (mounts[i].time_to_lock <= 0) &&        // if the mount has it locked
    TargetTracked()                         // if the target is being tracked
) {
    autotarg = target;                      // save it as the auto-target
}

// get the radar cone
float tracking_cone = radar.tracking_cone;
if (
    // is the target inside the tracking cone and close enough to target
    CloseEnoughToAutotrack(this, target, tracking_cone)
) { 
   // if there's an auto-target already...
   if (autotarg) {
        // then check against the radar tracking cone???
       if (radar.tracking_cone < tracking_cone) {
            // and update the tracking cone...
           tracking_cone = radar.tracking_cone;
        }
   }    
   // auto-select the target since it's close enough...
   autotarg = target;
}

Seems like a few things could be cut down to a single check, or at least the perspective should be changed - like doing this on the individual mounts against their views and distances. (The first half)

The updating of the tracking cone and auto-target in the second portion seems problematic at the very least.

[stephengtuggy]

Yeah, this code is not the easiest to read. I've seen other places in the code where "assignments" are used in unclear ways too. Like in the Mesh-related stuff. Very confusing. In many cases, I can't figure it out at all, and could use some help from long-term project members to understand the code.

[stephengtuggy]

At first, I thought that autotarg was being used to "push" and "pop" the existing value of target at the beginning and end of this block. But on further inspection, it doesn't look like that's the case.

[stephengtuggy]

My guess would be that at least one of the functions called here has side effects. Ugh.