Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object3D: Add childadded and childremoved events. #16934

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jun 27, 2019

Reviving some of the ideas from #11459 this PR adds childadded and childremoved events that get dispatched from the parent when a child is added or removed.

I'd like to track when an object is added or removed from the scene anywhere in the hierarchy and there's currently no simple way to achieve that.

I can add the tests and docs if this looks merge-able.

@gkjohnson gkjohnson changed the title Add "addchild" and "removechild" events to Object3D Add "childadded" and "childremoved" events to Object3D Jun 27, 2019
@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 28, 2019

I'd like to track when an object is added or removed from the scene anywhere in the hierarchy and there's currently no simple way to achieve that.

In this case, #8368 sounds more appropriate, right? I mean this PR only allows to track a removal or addition of direct child objects.

@D504
Copy link

D504 commented Jun 28, 2019

@Mugen87, do you think it has to work recursively?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Jun 28, 2019

@Mugen87

In this case, #8368 sounds more appropriate, right?

This PR is a lot simpler, introduces a useful new event notification and gives a lot of the same capability without the potential overhead of recursively firing events on every object in deep hierarchies.

I mean this PR only allows to track a removal or addition of direct child objects.

It does but it can be used to track every object addition and removal to and from the scene (or sub hierarchy) pretty easily. I've hacked this event into my application and am tracking object addition and removal like so (this is a simplified example):

    const allObjects = new Set();
    let addChildCb, removeChildCb;
    addChildCb = function(e) {
        const child = e.child;
        child.traverse(c => {
            c.addEventListener('childadded', addChildCb);
            c.addEventListener('childremoved', removeChildCb);
            allObjects.add(c);
        });
    };

    removeChildCb = function(e) {
        const child = e.child;
        child.traverse(c => {
            c.removeEventListener('childadded', addChildCb);
            c.removeEventListener('childremoved', removeChildCb);
            allObjects.delete(c);
        });
    };

    addChildCb({ child: scene });

The only other way to really know this right now is to manually track the addition of every object or traverse the hierarchy every frame and look for differences. I think there's a lot of utility to easily and efficiently tracking which objects have been added to the world.

@EliasHasle
Copy link
Contributor

Do the extra events provide anything that cannot be achieved by accessing the parent of the target object of the added/removed events?

@gkjohnson
Copy link
Collaborator Author

The example I showed in the comment above cannot be achieved with just the add and remove events. This allows a non-invasive and complete approach to tracking all of the additions and removals from a scene by notifying when children are changed. The add and remove event can't be used because you need to know about the child in order to add the event, which can't always be done because different libraries/ loaders or non-modifiable user code may add objects into the scene in an untrackable way.

My request for global uniforms (#16922) would be addressed by this PR because all Meshes with materials could be tracked and have their uniforms updated before render (traverse is slow and iterates over a lot of non-mesh objects). I feel in a lot of ways this allows a lot of flexibility and for features to be transparently added without the need for modifying core.

@EliasHasle
Copy link
Contributor

EliasHasle commented Jul 3, 2019

I am not sure I get it. Why can't you use the event target like this? :

const allObjects = new Set();
let addCb, removeCb;
addCb = function(e) {
    const child = e.target;
    child.traverse(c => {
        if (c !== child) {
            c.addEventListener('added', addCb);
            c.addEventListener('removed', removeCb);
            allObjects.add(c);
        }
    });
};

removeCb = function(e) {
    const child = e.target;
    child.traverse(c => {
        if (c !== child) {
            c.removeEventListener('added', addCb);
            c.removeEventListener('removed', removeCb);
            allObjects.delete(c);
        }
    });
};

allObjects.add(scene);
addCb({ target: scene });

For any cases where you need access to the parent, it could perhaps be an idea to have a parent property in the 'added' and 'removed' events? (In 'removed' it cannot otherwise be retrieved, because the event is dispatched after the removal is complete.)

@gkjohnson
Copy link
Collaborator Author

@EliasHasle
Your solution does not allow for tracking additions of children to the scene, which is what I'm after. Please try it for yourself:

Using just the add and remove events the way you proposed:

const scene = new THREE.Scene();
allObjects.add(scene);
addCb({ target: scene });
scene.add(new THREE.Object3D());

console.log(Array.from(allObjects.values()));
// logs [Scene] but we expect [Scene, Object3D]

And using the childadded and childremoved events I'm proposing:

const scene = new THREE.Scene();
addChildCb({ child: scene });
scene.add(new THREE.Object3D());

console.log(Array.from(allObjects.values()));
// logs [Scene, Object3D]

@EliasHasle
Copy link
Contributor

Sorry. You are totally right. Maybe the 'childadded' and 'childremoved' events are generally more useful than the 'added' and 'removed' events? What are really the use cases for the existing ones, that cannot be covered by the proposed ones?

@IRobot1
Copy link

IRobot1 commented Jun 10, 2023

I have some code that horizontally or vertically positions children within a group based on sort order defined in userData. Without these events, I have to check if children.length has changed each frame to trigger if this positions need recalculating.
Please consider adding this change. It would be really helpful.
It doesn't need to be recursive. Each group can notify any parent group with a childchanged event if needed.

@IRobot1
Copy link

IRobot1 commented Jun 10, 2023

Without this change, every Object3D must listen for added and removed events...

class Thing extends Object3D {
  constructor() {
    super();
    this.addEventListener('added', () => {
      console.log('added', this.parent);
      this.parent?.dispatchEvent({ type: 'childadded', child: this });
    });

    this.addEventListener('removed', () => {
      console.log('removed', this.parent);
      this.parent?.dispatchEvent({ type: 'childremoved', child: this });
    });

  }
}

const group = new Group();
group.addEventListener('childadded', (event: any) => { console.log('child added', event) })
group.addEventListener('childremoved', (event: any) => { console.log('child removed', event) })

const thing = new Thing();
group.add(thing);
group.remove(thing);

Unfortunately, Object3D.remove sets parent to null before calling dispatchEvent, so the childremoved event never fires

object.parent = null;
this.children.splice( index, 1 );

object.dispatchEvent( _removedEvent );

This can be worked around by storing parent during the add, but its such a hack

  constructor() {
    super();
    let parent: Object3D | undefined = undefined;

    this.addEventListener('added', () => {
      console.log('added', this.parent);
      this.parent?.dispatchEvent({ type: 'childadded', child: this });
      parent = this.parent;
    });

    this.addEventListener('removed', () => {
      console.log('removed', this.parent);
      parent?.dispatchEvent({ type: 'childremoved', child: this });
    });

@artificial-jon
Copy link
Contributor

What is the prognosis for this making it in? It's a very safe and simple change, and I've had multiple instances where this functionality would be useful.

@AdamEisfeld
Copy link

AdamEisfeld commented Jan 16, 2024

+1 to this, I didn't see this PR and had created this issue outlining the same solution Issue 27563.

With the childadded / removed events, we can then intercept the addition /removal of objects to / from a Scene (via a subclass or a wrapper) and recursively set the childadded / childremoved events on every object to dispatch some event on the scene (eg "scenegraph_child_added" / "scenegraph_child_removed").

Then we can observe these scenegraph_child_added / scenegraph_child_removed events to perform some logic (eg, inspecting the class of the added object / any child objects and, if they pass some test, adding them to a list of objects to track / update each render loop).

@AdamEisfeld
Copy link

An alternative approach would be to dispatch the old added / removed events globally via the document. If the new parent / old parent objects were added as parameters to this event, outside observers could listen for these events and then simply traverse the parent chain up to see if it ends on a Scene object in order to check if the scene graph has changed.

This would be a breaking change though, as users would need to start monitoring the document for the added / removed events instead of a specific object. The benefit being it would stick to just using the one event instead of adding a new one.

@Mugen87 Mugen87 added this to the r162 milestone Feb 16, 2024
@Mugen87 Mugen87 changed the title Add "childadded" and "childremoved" events to Object3D Object3D: Add childadded and childremoved events. Feb 16, 2024
@@ -357,6 +357,7 @@ Object3D.prototype = Object.assign( Object.create( EventDispatcher.prototype ),
this.children.push( object );

object.dispatchEvent( _addedEvent );
this.dispatchEvent( { type: 'childadded', child: object } );
Copy link
Collaborator

@Mugen87 Mugen87 Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #11459 (comment).

@gkjohnson I think it's okay creating an object per event but I also understand the concerns of devs who perform many add and remove operations. A single object would be more performant.

Since the listeners in dispatchEvent() are processed in a sync fashion, I think it would be safe to have single event objects and just change the child reference. Besides, other event types like added or removed have shared event objects as well. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good - we just have to set and then remove the objects from the cached event before and after dispatch. I'll merge this as is and then make a new PR with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants