Regression in component initialization

[mrxz]

Description: Commit 924dc00ecf483c1cd21c01bcadaf53d95911155d changed the initialization behaviour for components. It postpones setting hasLoaded on an entity after its components have initialized. However, this introduces a state where hasLoaded is false, but the component is supposed to initialize. The updated guard if(!el.hasLoaded && !el.sceneEl) (component.js#L279-L281) can't distinguish between a legitimate initialization and one triggered by a setAttribute (e.g. one originating from the init or update of another component).

This can result in component initialization taking place before it should, followed by an update from updateComponents. In case of primitives this also means the initialization might lack properties.

Glitch demonstrating an issue arising from this change in behaviour: https://glitch.com/edit/#!/mint-available-newsboy?path=index.html (The error itself is a different bug, but note that it surfaced due to a change in behaviour caused by the mentioned commit)

Personally I'm not entirely sure what issue the commit in question tries to resolve. The old behaviour was clearly intended, as is also evident from the comment in component.js which still indicates that component initialization follows after the entity has loaded. Components and experiences will already have been written assuming this behaviour. For cases where a component depends on other components on the same entity, the dependencies array can be used or the component can ensure it's dependencies are met like how the various controls handle it (e.g. oculus-touch-controls -> tracked-controls -> tracked-controls-webvr/xr)

• A-Frame Version: master
• Platform / Device: any
• Reproducible Code Snippet or URL: https://glitch.com/edit/#!/mint-available-newsboy?path=index.html

[dmarcos]

Sorry, I should have better documented the fix. The bug manifested when calculating bounding boxes here:

https://github.com/aframevr/aframe/blob/master/src/components/obb-collider.js#L66

The geometry component was not initialized when the hasLoaded flag was true and the bounding box initialized to zero. Notice the obb-collider component doesn't depend on the geometry component directly (or any others that might set a mesh). An entity with an obb-collider could have a gltf-model component and not a geometry one. Just want to know that all components have initialized before calculating the bounding box.

I myself always assumed that components would be ready after the loaded event fires (and corresponding hasLoaded flag set) as stated in docs

Whether the entity has attached and initialized all of its components

What would it mean that an entity has loaded if components have not also initialized?

[dmarcos]

We have to fix all the kinks

[dmarcos]

If can generate some tests where the new code fails I'm happy to look at it.

[mrxz]

The geometry component was not initialized after the 'loaded' event fired and the bounding box size was zero. Notice the component doesn't depend on the geometry directly (or any others that might set a mesh). Just need to know the components have initialized before performing the calculation.

That does indeed illustrate the reason for the change, but isn't it now using the hasLoaded and loaded event as a proxy to know when the geometry component initialized? In fact, all it should really care about is updating the bounding box during initialization and upon each change to the underlying mesh(es).

Listening for object3dset and object3dremove should probably do the trick. This eliminates the need to specifically listen to the model-loaded event, handles cases when the geometry component is added or changed post-initialization, and where child entities change their mesh(es).

What would it mean that an entity has loaded if components have not also initialized?

The guarantee that the loaded state of an entity currently gives pertains to its children. An entity is loaded iff assets have loaded and child entities have emitted the loaded event (components initialized). This has the neat property that there's exactly one state transition, after which the entity is ready to initialize and/or update its components. This is regardless of when/where these component inits/updates come from, whether they were already set on the entity pre-load or dynamically set afterwards.

By changing this semantic, there are now three states: not loaded, initializing, loaded. That is also where the mentioned regression stems from, the code doesn't properly distinguish the first two states in all cases, leading to components initializing while the entity is in effect not loaded and not yet initializing its components.

If a component should robustly handle initialization ordering issues, the updated behaviour of hasLoaded/loaded event is of limited use, as it doesn't provide a solution in case the component is dynamically added post-load. In practice it's generally possible to find event-driven approaches that work regardless if its the initial initialization of the entity or not.

That being said, the documentation differing from how it works and is documented in code is far from ideal. And the fact that the hasLoaded flag can be observed true before the loaded event (which also provides additional guarantees) is a potential pitfall. I'm just not sure that the added complexity and change in behaviours is worth it. In fact, I don't think components should ever care whether they are initialized as part of the "initial entity initialization" or "post loaded".

[dmarcos]

Listening for object3dset and object3dremove should probably do the trick. This eliminates the need to specifically listen to the model-loaded event, handles cases when the geometry component is added or changed post-initialization, and where child entities change their mesh(es)

It's not enough. there are many other factors that affect the size of a bounding box. e.g adding children entities. I want to run the bounding box calculation after all components have loaded because cannot anticipate all scenarios before hand. I covered model-loaded as well since loading a model is async and loading a model common. I can make more granular and robust the logic over time but making sure the bounding box calculation runs after components have loaded will catch a lot of scenarios without ad-hoc logic.

Definitively my intent with the loaded event and that the hasLoaded flag is to indicate that the entity and components have loaded.

If you produce some simple examples or tests that fail with the new logic and not the old one I'll be happy to fix.

[mrxz]

It's not enough. there are many other factors that affect the size of a bounding box. e.g adding children entities. I want to run the bounding box calculation after all components have loaded because cannot anticipate all scenarios before hand.

I was under the impression that object3dset and object3dremove did a bit more heavy lifting, given its usage in the raycaster. While they do properly report child entities being added and models being loaded, you're right that they have severe shortcomings when it comes to removal of child entities or geometry changes.

However, it still feels like an XY problem in the context of this component. IMHO the component shouldn't care about entity initialization, but simple compute the bounding box and recompute upon each change (possible batched using a dirty flag). This way there is only one scenario to think about, nothing ad-hoc needed. The fact that there is no combination of events to easily achieve this, is potentially worth addressing. But that's beside the point of this issue.

Definitively my intent with the loaded event and that the hasLoaded flag is to indicate that the entity and components have loaded.

Having hasLoaded and the loaded event in agreement is good. With the entity loading state becoming a tri-state, modelling it as such will likely make the code easier to read and debug.

If you produce some simple examples or tests that fail with the new logic and not the old one I'll be happy to fix.

Here's a simple example where the resulting plane should be square (as it is in 1.5.0 and prior), but ends up elongated on master: https://glitch.com/edit/#!/puddle-awesome-coal?path=index.html

Here's an example where the removal of a mixin no longer takes effect: https://glitch.com/edit/#!/petal-absorbing-viscountess?path=index.html

Here's an example where an early initialization (triggered by .setAttribute) results in an additional update, which doesn't occur with 1.5.0: https://glitch.com/edit/#!/mixed-orange-raven?path=index.html

And of course the example with aframe-environment-component in the opening post. Although with #5426 merged, the erroneous behaviour now no longer results in an error. Though I believe it has the same cause as the first example.