Gamua / Starling-Framework

The Cross Platform Game Engine
http://www.starling-framework.org
Other
2.82k stars 821 forks source link

requiresRedraw not always properly validated #967

Closed b005t3r closed 7 years ago

b005t3r commented 7 years ago

I have this class:

public class DevBackgroundSprite extends BackgroundSprite {
    public static const PARENT_X_MARGIN:Number          = 26;
    public static const PARENT_Y_MARGIN:Number          = 26;
    public static const BORDER_SIZE:Number              = 2.5;
    public static const LINE_SIZE:Number                = 3.5;
    public static const INTERNAL_X_MARGIN:Number        = 14;
    public static const INTERNAL_Y_MARGIN:Number        = 12;
    public static const INTERNAL_SPACING:Number         = 2;

    public static const TITLE_FONT_SIZE:Number          = 18;
    public static const SUBTITLE_FONT_SIZE:Number       = 12;
    public static const SPECIAL_RULES_FONT_SIZE:Number  = 12;
    public static const STANDARD_RULES_FONT_SIZE:Number = 12;

    public static const WIDTH:Number            = DevCardSprite.WIDTH - 2 * PARENT_X_MARGIN;
    public static const HEIGHT:Number           = DevCardSprite.HEIGHT - 2 * PARENT_Y_MARGIN;

    private var _bgMesh:Mesh;
    private var _fgMesh:Mesh;
    private var _lineMesh:Mesh;

    private var _title:TextField;
    private var _subtitle:TextField;
    private var _specialRules:TextField;
    private var _standardRules:TextField;

    public function DevBackgroundSprite() {
        super(WIDTH, HEIGHT);

        var poly:Polygon = Polygon.createRectangle(0, 0, WIDTH, HEIGHT);

        if(poly == null)
            throw new ArgumentError("poly cannot be null");

        var bgVertexData:VertexData = new VertexData();
        var fgVertexData:VertexData = new VertexData();
        var lineVertexData:VertexData = new VertexData();
        var indexData:IndexData = new IndexData(poly.numTriangles * 3);

        poly.triangulate(indexData);
        poly.copyToVertexData(bgVertexData);
        poly.copyToVertexData(fgVertexData);
        poly.copyToVertexData(lineVertexData);

        bgVertexData.colorize("color", Color.BLACK, 1.0);
        fgVertexData.colorize("color", Color.WHITE, 1.0);
        lineVertexData.colorize("color", Color.BLACK, 1.0);

        _bgMesh = new Mesh(bgVertexData, indexData);
        _fgMesh = new Mesh(fgVertexData, indexData);
        _lineMesh = new Mesh(lineVertexData, indexData);

        _fgMesh.width   -= 2 * BORDER_SIZE;
        _fgMesh.height  -= 2 * BORDER_SIZE;
        _fgMesh.x       += BORDER_SIZE;
        _fgMesh.y       += BORDER_SIZE;

        _lineMesh.height = LINE_SIZE;
        _lineMesh.y      = (HEIGHT - LINE_SIZE) / 2;

        addChild(_bgMesh);
        addChild(_fgMesh);
        addChild(_lineMesh);

        _title                                  = new DevTextField(WIDTH - 2 * INTERNAL_X_MARGIN, 1, "Sample title");
        _title.autoSize                         = TextFieldAutoSize.VERTICAL;
        _title.format.horizontalAlign           = Align.RIGHT;
        _title.format.size                      = TITLE_FONT_SIZE;
        _title.x                                = WIDTH - INTERNAL_X_MARGIN;
        _title.y                                = INTERNAL_Y_MARGIN;
        _title.alignPivot(Align.RIGHT, Align.TOP);

        _subtitle                               = new DevTextField(WIDTH - 2 * INTERNAL_X_MARGIN, 1, "Sample subtitle");
        _subtitle.autoSize                      = TextFieldAutoSize.VERTICAL;
        _subtitle.format.horizontalAlign        = Align.RIGHT;
        _subtitle.format.size                   = SUBTITLE_FONT_SIZE;
        _subtitle.x                             = WIDTH - INTERNAL_X_MARGIN;
        _subtitle.alignPivot(Align.RIGHT, Align.TOP);

        _specialRules                           = new DevTextField(WIDTH - 2 * INTERNAL_X_MARGIN, 1, "Sample special rules");
        _specialRules.autoSize                  = TextFieldAutoSize.VERTICAL;
        _specialRules.format.horizontalAlign    = Align.LEFT;
        _specialRules.format.size               = SPECIAL_RULES_FONT_SIZE;
        _specialRules.x                         = INTERNAL_X_MARGIN;
        _specialRules.y                         = HEIGHT / 2 + INTERNAL_Y_MARGIN;
        _specialRules.alignPivot(Align.LEFT, Align.TOP);

        _standardRules                          = new DevTextField(WIDTH - 2 * INTERNAL_X_MARGIN, 1, "Sample standard rules");
        _standardRules.autoSize                 = TextFieldAutoSize.VERTICAL;
        _standardRules.format.horizontalAlign   = Align.LEFT;
        _standardRules.format.size              = STANDARD_RULES_FONT_SIZE;
        _standardRules.x                        = INTERNAL_X_MARGIN;
        _standardRules.alignPivot(Align.LEFT, Align.TOP);

        addChild(_title);
        addChild(_subtitle);
        addChild(_specialRules);
        addChild(_standardRules);
    }

    override public function get title():DisplayObject { return _title; }
    override public function get subtitle():DisplayObject { return _subtitle; }
    override public function get specialRules():DisplayObject { return _specialRules; }
    override public function get standardRules():DisplayObject { return _standardRules; }

    override public function render(painter:Painter):void {
        layout();

        super.render(painter);
    }

    private function layout():void {
        if(! _title.requiresRedraw && ! _subtitle.requiresRedraw && ! _specialRules.requiresRedraw && ! _standardRules.requiresRedraw)
            return;

        _subtitle.y         = _title.y + INTERNAL_SPACING + _title.height;
        _standardRules.y    = _specialRules.y + INTERNAL_SPACING + _specialRules.height;
    }
}

Take a look at the layout() method. It's called on each render() call and requiresRedraw for each child object is evaluated then.

Now, if this display object is made visible and added to stage, everything works as expected - requiresRedraw is false as it should be. BUT if this object is hidden and made visible on one of the subsequent frames (after it has been added to stage) requiresRedraw returns false which is obviously not the expected behaviour, because these objects haven't been rendered yet at all.

b005t3r commented 7 years ago

I gave setRequiresRedraw() a quick look and I'm not sure if how it's implemented makes sense:

public function setRequiresRedraw():void
{
    var parent:DisplayObject = _parent || _maskee;
    var frameID:int = Starling.frameID;
    _lastParentOrSelfChangeFrameID = frameID;
    _hasVisibleArea = _alpha  != 0.0 && _visible && _maskee == null && _scaleX != 0.0 && _scaleY != 0.0;

    while (parent && parent._lastChildChangeFrameID != frameID)
    {
        parent._lastChildChangeFrameID = frameID;
        parent = parent._parent || parent._maskee;
    }
}

If an object requires to a redraw, it sets all ancestors to require a redraw as well, but I think it should do this for all descendants as well. With the current implementation they are going to be redrawn anyway, but this should be reflected in their internal state as well (reuiresRedraw should return true for such cases).

PrimaryFeather commented 7 years ago

Starling marks the children as requiring a redraw only during rendering: otherwise, the logic of marking objects "dirty" would be very very expensive. Think of it: when you make a change to your root object, all of the children would need to be iterated over.

On rendering, on the other hand, we iterate through all objects anyway, so it's almost free to add that logic there.

This means that requiresRedraw only indicates if the object or one of its children was changed, but does not take any parents into account. You could easily do that manually, though, by checking that property for all its parents, too.

Do you see what I mean?

b005t3r commented 7 years ago

Do you see what I mean?

Yeah, I've had this feeling it could hurt performance. It's still not ideal as it is currently, because checking if a given object will be redraw requires checking all of its ancestors, but I understand why it has to be this way.

PrimaryFeather commented 7 years ago

Yeah, I did not really like it either, but there were some compromises I had to make in order to have the render cache not be too expensive in those cases where the display list changes.