1
Vote

Bug in ordering sheets

description

Hello, we would like to inform you about a bug we found in the code. The problem resides in function getOrderedList(). We run into a problem if nothing has changed in an iteration of for( ;; ) loop. There is the following code fragment (lines 1259-1273):
if (no change) {
    var zmax = -10000;
    for (var i=0; i<unordered.length; i++) {
        if ((i == 0) || (sheetengine.polygons[unordered[i]].data.zmax > zmax)) {
            maxidx = i;
            zmax = sheetengine.polygons[unordered[i]].data.zmax;
        }
    }
    ...
}
There are two bugs, in fact. The first one is a missing declaration of "maxidx" which is then considered global and its value persists to subsequent function calls. The other one is untreated special condition, when there is exactly one item in "unordered" and (who knows?) its data.zmax has default value of -10000. Such a condition results in an indefinite loop. Or, if maxidx declaration is present, it results in a crash in drawSheets, because orderedPolygons contains "undefined" item. We suggest to apply following change:
if (nochange) {
    var zmax = -10000;
    var maxidx = -1;
    for (var i=0; i<unordered.length; i++) {
        if ((i == 0) || (sheetengine.polygons[unordered[i]].data.zmax > zmax)) {
            maxidx = i;
            zmax = sheetengine.polygons[unordered[i]].data.zmax;
        }
    }

    var key = 'k'+unordered[maxidx];
    ordered[key] = unordered[maxidx];
    //ordered[ordered.length] = unordered[maxidx];
    unordered.splice(maxidx, 1);
    if (unordered.length == 0)
        break;
}

comments

dobsonl wrote May 1, 2014 at 6:37 PM

Thanks for the report. This is definitely a bug.

RohanDeshpande wrote Sep 4, 2014 at 2:04 AM

Levente, have you tested out this fix? Do you think it's good to include it in the source?

dobsonl wrote Sep 4, 2014 at 5:42 AM

No, I haven't tested it, but looks ok to me.