bp74 / StageXL

A fast and universal 2D rendering engine for HTML5 and Dart.
http://www.stagexl.org
Other
880 stars 82 forks source link

DisplayObject._getCommonAncestor not null-safe #344

Closed QuaverJosh closed 2 years ago

QuaverJosh commented 3 years ago

Consider the following use case:

final item = Sprite()
 ..addTo(stage!);

final tempHighlight = Shape()
  ..graphics.rect(0, 0, 100, 100)
  ..graphics.strokeColor(Color.Black, 4);

final highlightTexture = BitmapData(tempHighlight.width, tempHighlight.height, Color.Transparent)
   ..draw(tempHighlight, tempHighlight.getTransformationMatrix(item.stage!));

When run, this will throw a runtime unexpected null error due to obj1 being set to null during the first while loop:

DisplayObject _getCommonAncestor(DisplayObject other) {
    var obj1 = this;
    var obj2 = other;
    var depth1 = 0;
    var depth2 = 0;

    for (var o = obj1; o.parent != null; o = o.parent) {
      depth1 += 1;
    }
    for (var o = obj2; o.parent != null; o = o.parent) {
      depth2 += 1;
    }
    while (depth1 > depth2) {
      obj1 = obj1.parent;
      depth1 -= 1;
    }
    // ignore: invariant_booleans
    while (depth2 > depth1) {
      obj2 = obj2.parent;
      depth2 -= 1;
    }

    while (identical(obj1, obj2) == false) {
      obj1 = obj1.parent;
      obj2 = obj2.parent;
    }

    return obj1;
  }

You can get around this error by ensuring tempHighlight.addTo(stage!) is called before calling tempHighlight.getTransformationMatrix(item.stage!); but I believe the correct place for these edits is in the _getCommonAncestor method itself, by allowing obj1 and obj2 to be nullable:

  DisplayObject? _getCommonAncestor(DisplayObject other) {
    DisplayObject? obj1 = this;
    DisplayObject? obj2 = other;
    var depth1 = 0;
    var depth2 = 0;

    for (var o = obj1; o.parent != null; o = o.parent!) {
      depth1 += 1;
    }
    for (var o = obj2; o.parent != null; o = o.parent!) {
      depth2 += 1;
    }
    while (depth1 > depth2) {
      obj1 = obj1?.parent;
      depth1 -= 1;
    }
    while (depth2 > depth1) {
      obj2 = obj2?.parent;
      depth2 -= 1;
    }

    while (identical(obj1, obj2) == false) {
      obj1 = obj1?.parent;
      obj2 = obj2?.parent;
    }

    return obj1;
  }

This removes the requirement that tempHighlight be added to the stage before getTransformationMatrix is called, which matches the behavior of previous versions of StageXL.

AndrewLugg commented 2 years ago

I have ran into this same issue, that we had some items checked to see if they had a common parent before being added to the stage, to check for collisions. It shouldn't happen but it would be good if it didn't throw a null safe error. Even throwing an exception of no same parent would be good.

AndrewLugg commented 2 years ago

I have applied this fix to the repository, and will upload to pub soon.