Gamua / Starling-Framework

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

Potential issue with broadcastEvent() #894

Open b005t3r opened 8 years ago

b005t3r commented 8 years ago

This situation I'm describing here caused problems with my project, where I copied your event system, Daniel. It probably doesn't have any effect on Starling currently, but it may cause problems in the future.

In DisplayObjectContainer you have two methods: broadcastEvent() and getChildEventListeners(). What they do is firing a given event for the parent and each children, but they always do it in the same order - parent is always first, children are always second. This is OK for event like ADDED_TO_STAGE, but is backward for REMOVED_FROM_STAGE where the parent notifies the rest of the system that it has been removed and the children still haven't done that. For me it was an issue, so I fixed it by adding a bottomUp parameter to both of this methods and changed getChildEventListeners() like so:

    internal function getChildEventListeners(object:Node, eventType:String, listeners:Vector.<Node>, bottomUp:Boolean):void {
        var container:ContainerNode = object as ContainerNode;

        if(! bottomUp && object.hasEventListener(eventType))
            listeners[listeners.length] = object; // avoiding 'push'

        if(container) {
            var children:Vector.<Node> = container._nodes;
            var numChildren:int = children.length;

            for(var i:int = 0; i < numChildren; ++i)
                getChildEventListeners(children[i], eventType, listeners, bottomUp);
        }

        if(bottomUp && object.hasEventListener(eventType))
            listeners[listeners.length] = object; // avoiding 'push'
    }

so it reverses the order listeners are gathered if bottomUp equals true, which is when a container is removed from the stage.

As I said, probably not a problem at the moment, but a logical change which should break anything. Give it a thought.

PrimaryFeather commented 8 years ago

Thanks for the suggestion! You're right, this might be a good idea. I can't remember 100%, but I think when I implemented this I compared it to classic Flash, and it worked just like my implementation, and that's why I left it at that. — but it's been a while, maybe I'm mixing things up.

b005t3r commented 8 years ago

As I said, probably not an issue currently, but might be a good idea to change it before it becomes an issue. In my framework (https://github.com/b005t3r/Stork-Framework) I have this dependency injection thing implemented and in a very specific set scenarios, with deep node hierarchies I had an issue where a given node was already removed from the stage, but it's children still though they are part of the stage and the whole dependency injection mechanism was not working properly. It's a bit specific, hard to explain and I don't think I can trace the exactly cause (it's really complex - some node unregistered, some still registered, some trying to inject dependencies from the stage which is not possible at this point), but this particular change fixed the issue for me and really makes sense, if you think about it (that's how constructors and destructors work in C++, for example).