aframevr / aframe

:a: Web framework for building virtual reality experiences.
https://aframe.io/
MIT License
16.64k stars 3.96k forks source link

Consider stricter lifecycle guarantees #3556

Open donmccurdy opened 6 years ago

donmccurdy commented 6 years ago

Got bit recently with an unexpected issue writing a component, thought I'd write it up for ideas...

The nav-mesh component needs the mesh from the entity it's attached to, and needs to account for transformations on its parents. I'd imagined this would be simple enough:

AFRAME.registerComponent('nav-mesh', {
  init: function () {
    this.loadNavMesh();
    this.el.addEventListener('object3dset', () => this.loadNavMesh());
  },

  loadNavMesh: function () {
    const navMesh = this.el.getObject3D('mesh');

    if (!navMesh) return;

    const navMeshGeometry = navMesh.geometry.isBufferGeometry
      ? new THREE.Geometry().fromBufferGeometry(navMesh.geometry)
      : navMesh.geometry.clone();

    scene.updateMatrixWorld();
    navMeshGeometry.applyMatrix(navMesh.matrixWorld);
    this.system.setNavMeshGeometry(navMeshGeometry);
  }
});

Unfortunately this fails — when init runs, the entity's parents do not have their position/rotation/scale applied yet, so scene.updateMatrixWorld() does nothing, and the transformation isn't applied to the Geometry.

Next I assumed update() would be a safe place to put the loadNavMesh() call, but it too runs before parents are initialized. I don't want this code to run on play (it shouldn't reload when the scene pauses) so instead I ended up with:

tick: function (t) {
  if (t === 0) this.loadNavMesh();
}

Anyway, all of this was a little more complicated than I expected, and harder than component lifecycles in e.g. Angular. Saw this issue (or similar) called out as a pain point compared to Unity recently, too:

Have to wrestle with asynchronicity issues that you generally don’t run into with Unity (e.g. why is this model loading BEFORE something else it is dependent on …)

Anyway, I don't have a concrete proposal at the moment but wanted to think about tightening up component lifecycles in a future version and documenting clearly what component authors should expect.

dmarcos commented 6 years ago

Initialization order is from children to parents. When init and update run for an entity all of its children have initialized but not the other way around. You could listen to the loaded event on the parent to get initialized. Asynchronicity is more a general problem of the Web than specific to A-Frame due to the single threaded JS context.

donmccurdy commented 6 years ago

I think the problem is more complex than that. Using the loaded event gets complicated when you need to consider:

  1. component may be attached before or after scene has loaded
  2. component may be attached before or after entity has loaded
  3. component may be attached before or after entity's mesh has loaded
  4. component itself may have async behavior

I don't see asynchronicity as a problem, given ways to direct async program flow, but currently I do some guessing and rely on undocumented behavior that can and has changed over time, like exactly when update will initially run. It's overkill, but see the much more granular initialization hooks that Angular provides:

https://angular.io/guide/lifecycle-hooks

They also provide a clearly-documented order for all hooks:

peek-a-boo

I don't know React or Unity, but I believe they have some additional controls here as well: https://docs.unity3d.com/Manual/ExecutionOrder.html

The fact that I can't know the initial position of my entity until the first tick has already begun seems like a hazard. I don't think we should rush into any changes here, but I do think it's worth considering whether we can make things more predictable for component authors.

ngokevin commented 6 years ago

You could move the component up the tree, or run it on play with a check that it only runs once, run it on scene loaded, or manage it in a system. It's fairly common even in web development to write event listeners to wait for something to happen first (e.g., DOMContentLoaded).

The fact that I can't know the initial position of my entity until the first tick has already begun seems like a hazard.

Seems like you want to know the position of the parent entity, no? The position component initializes first if present.

We could add more hooks if we need, but wrote like apps with dozens of components, haven't run into something where there wasn't a clean solution (usually using events).

I think we have an issue open for allowing dependencies on components with an event (e.g., depend on model loaded), though can be accomplished with event listener, haven't felt too much need to add more API.

Any specific improvements or areas of documentation suggestions welcome.

On Tue, Apr 24, 2018 at 10:19 PM Don McCurdy notifications@github.com wrote:

I think the problem is more fundamental than that. Using the loaded event is a pain, and gets complicated when you need to consider:

  1. component may be attached before or after scene and entity have loaded
  2. component may be attached before or after entity's mesh has loaded
  3. component itself may have async behavior

I don't see asynchronicity as a problem, given the ability to control program flow, but currently I do some guessing and rely on undocumented behavior that can and has changed over time, like exactly when update will initially run. It's overkill, but see the much more granular initialization hooks that Angular provides:

  • ngOnInit
  • ngAfterContentInit
  • ngAfterContentChecked
  • ngAfterViewInit
  • ngAfterViewChecked
  • ngOnChanges

https://angular.io/guide/lifecycle-hooks

They also provide a clearly-documented order for all hooks:

[image: peek-a-boo] https://user-images.githubusercontent.com/1848368/39227377-26c6083a-480d-11e8-98ff-b4982c0d4372.png

I don't know React or Unity, but I believe they have some additional controls here as well: https://docs.unity3d.com/Manual/ExecutionOrder.html

I don't think we should rush into any changes here, but I do think it's worth considering whether we can make things more predictable for component authors.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/aframevr/aframe/issues/3556#issuecomment-384163696, or mute the thread https://github.com/notifications/unsubscribe-auth/AApLpx8cbu1Mtkn_xITu3nUDeGUfvnZMks5tsAdzgaJpZM4TiupO .

--

Kevin Ngo ngokevin.com

dmarcos commented 6 years ago

@donmccurdy In your case above I think it would be sufficient to do:

this.el.sceneEl.addEventListener('loaded', () => this.loadNavMesh());

This way all the entities all the way up to a-scene (that can influence the position of your entity) are initialized.

If you can describe other scenarios we might offer solutions with the current APIs. We could then collect the best practices in the docs.

dmarcos commented 6 years ago

Oh I see. You might need to do in this.loadNavMesh:

 init : function () {
    this.el.addEventListener('object3dset', () => this.loadNavMesh());
 },

  loadNavMesh: function () {
    var sceneEl = this.el.sceneEl;
    // Postpone if scene has not loaded
    if (!sceneEl.hasLoaded) { 
      sceneEl.addEventListener('loaded', () => this.loadNavMesh());
     return;
   }
   ...
  }
PlumCantaloupe commented 6 years ago

This is a great discussion. Thanks for starting it.

I have run into similar issues; though admittedly it may be due to my coding style not completely being moved over to the appropriate style of js. I find the "loaded" event is a bit of a misnomer as it doesn't account for the actual content within the entity being loaded (i.e. a model and its associated textures/materials are not necessarily loaded as well). This may be where these issues are stemming from because any advanced functionality is diving into three.js/webgl layer and everything may not be set just yet.

When building a couple of components that accessed materials/meshes I had to approach similarly to Don's tick method. I see there is an "object3Dset" event I can listen to but that also doesn't necessarily mean the material is set correct?

Also, what about use cases where there are multiple dependencies within the ThreeJS layer. I am just getting to understand promises in js but how would we approach this within an asynchronous event-driven model (without having to resort to a bunch of questionable booleans in a tick)?

Question: Are some of these issues related to trying to maintain two areas where setting properties can take place: setAttribute(...) as position/scale/rotation as attributes vs. setting them directly to the object3D.position.set(...)?

e.g. of a rough component attempt I have been having troubles with (replacing the standard material with a basic material for my boundary wireframes). Sometimes shadows pop up (or not) depending on whether the parent entity (an avatar head with shadows enabled) loads before or after this child element (facial texture cutout material).

AFRAME.registerComponent('cutout-material', {
    schema: {
        src:            {type:'asset',   default:''},
        alphaTest:      {type:'number',  default:0.5},
        castShadow:     {type:'boolean', default:false},
        receiveShadow:  {type:'boolean', default:false},
        shading:        {type:'boolean', default:true}, //shading at al
        renderSide:     {type:'string',  default:'double'} //front, back, double
    },
    multiple: false, //do not allow multiple instances of this component on this entity
    init: function() {
        this.loader = new THREE.TextureLoader();
        this.applyMats();
        this.el.addEventListener('object3dset', this.applyMats.bind(this));
    },
    //custom function
    applyMats : function () {
        const Context_AF = this;
        const mesh = Context_AF.el.getObject3D('mesh');
        if (!mesh) return;

        if ( Context_AF.data.src === '' ) {
            console.warn('No src defined in component');
        } 
        else {
            Context_AF.loader.load( Context_AF.data.src,   
                function onLoad(texture) {
                    //texture.anisotropy = 16;

                    const customDepthMaterial = new THREE.MeshDepthMaterial( {
                        depthPacking: THREE.RGBADepthPacking,
                        map: texture,             
                        alphaTest: Context_AF.data.alphaTest  
                    });

                    const basicMaterial = new THREE.MeshBasicMaterial();

                    let renderSide = THREE.DoubleSide;
                    if ( Context_AF.data.renderSide === 'front' ) {
                        renderSide = THREE.FrontSide;
                    }
                    else if ( Context_AF.data.renderSide === 'back' ) {
                        renderSide = THREE.BackSide;
                    }

                    if (mesh) {
                        mesh.traverse(function (node) {

                            if (!Context_AF.data.shading) {
                                node.material = basicMaterial;
                            }

                            node.castShadow = Context_AF.data.castShadow;
                            node.receiveShadow = Context_AF.data.receiveShadow;

                            console.log(Context_AF.data.castShadow);
                            console.log(Context_AF.data.receiveShadow);

                            //if (node.material) {
                                //https://stackoverflow.com/questions/43848330/three-js-shadows-cast-by-partially-transparent-mesh?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa\
                                node.material.map           = texture;
                                node.material.side          = renderSide;
                                node.material.alphaTest     = Context_AF.data.alphaTest;
                                node.customDepthMaterial    = customDepthMaterial;

                                node.customDepthMaterial.needsUpdate    = true;
                                node.material.needsUpdate               = true;
                            //}
                        });
                    }

                    Context_AF.el.emit('materialset');
                },
                function onProgress(xmlHttpRequest) {
                    //xmlHttpRequest.total (bytes), xmlHttpRequest.loaded (bytes)
                },
                function onError(message) {
                    var message = (error && error.message) ? error.message : 'Failed to load texture';
                    console.log(message);
                }
            );
        }

    },
    //update: function () {},
    // tick: function(time, timeDelta) {},
    // tock: function(time, timeDelta) {},
    // remove: function() {},
    // pause: function() {},
    // play: function() {},
    // updateScheme: function(data) {}
});
ngokevin commented 6 years ago

I find the "loaded" event is a bit of a misnomer as it doesn't account for the actual content within the entity being loaded (i.e. a model and its associated textures/materials are not necessarily loaded as well).

An entity can always be loading things at any time, the loaded event is for loading all of its components and attaching onto the tree. Maybe it's better to use granular events and listen to what's needed than a catch-all loaded event that tries to consider every thing. I suppose we can introduce a way for components to hook into the loading process.

When building a couple of components that accessed materials/meshes I had to approach similarly to Don's tick method. I see there is an "object3Dset" event I can listen to but that also doesn't necessarily mean the material is set correct?

There's a materialtextureloaded event.

Also, what about use cases where there are multiple dependencies within the ThreeJS layer. I am just getting to understand promises in js but how would we approach this within an asynchronous event-driven model (without having to resort to a bunch of questionable booleans in a tick)?

The tick boolean isn't a recommended solution. three.js has callbacks anywhere where asynchronous, and from A-Frame, there are always events that can be used to hook into anywhere.

Are some of these issues related to trying to maintain two areas where setting properties can take place: setAttribute(...) as position/scale/rotation as attributes vs. setting them directly to the object3D.position.set(...)?

Not really, they do the same thing.

ngokevin commented 6 years ago

Perhaps components can have like a async: true property, and it has to call this.load() in order for the entity to be fully loaded. For like models. Though will cause inconsistency, some async components will adopt it, some won't.

It seems more explicit to wait for a modelloaded event versus a general loaded event. And it won't block further initializations and such.

PlumCantaloupe commented 6 years ago

Thanks for the response @ngokevin. And thanks for pointing to the 'materialtextureloaded' event - I must have missed that in the documentation. I suppose it gets a bit hairy trying to track down what events exist and what I can listen to before doing something - and if I need multiple things to exist/be set before doing something else.

I really like the idea of forcing some sort of synch on entity. It would be really handy, at least from those of us just jumping in and without the years of experience of knowing what hooks exist.

ngokevin commented 6 years ago

Yeah. It's actually probably better not to force it to allow other things to happen while a model is loading, and anything that needs that model listen to the specific event.

Did you try to look it up somehow? Or we just need to put a disclaimer somewhere to read events in component readme? Or describing the async nature somewhere? Hopefully we covered bases with:

Googling for example aframe listen texture and near beginning of material docs. It doesn't seem like that information is too far out of reach.

PlumCantaloupe commented 6 years ago

Well perhaps it is my own idiosyncrasies as it doesn't seem too far out of reach at all.

donmccurdy commented 6 years ago

Seems like you want to know the position of the parent entity, no? The position component initializes first if present.

You only have local position until play() or tick() is called. The entity could have N ancestors all with transforms, and to account for that you need their positions/scales/rotations to have been applied. World position is simply not available in init() or first update().

We could add more hooks if we need, but wrote like apps with dozens of components, haven't run into something where there wasn't a clean solution.

The points i'm describing here are easy to avoid when writing a particular app, but hard to do right if you want your component to work robustly in other people's scenes. E.g. many, many community components become buggy if you start adding parents with transforms around them.

this.el.sceneEl.addEventListener('loaded', () => this.loadNavMesh());

You're correct that this would work IF the component is never added dynamically, but again, I would be more comfortable doing this if lifecycle order were documented. I assume it's init -> update -> emit:loaded -> play -> tick? But it's easy to imagine update and loaded changing places, or loaded not being available at all for components added dynamically, and I don't want to rely on undefined behavior more than necessary.

Perhaps components can have like a async: true property, and it has to call this.load() in order for the entity to be fully loaded. For like models. Though will cause inconsistency, some async components will adopt it, some won't.

As an example, some of the most common questions and "bug reports" I get are related to users trying to do things before model-loaded or body-loaded have fired. I would gladly adopt async:true in these cases. Obviously there would be many questions about how that would work (does it delay traversing up the scene, or just block first render like a-assets?).

But, I'm not suggesting we rush into any changes here — especially not before 0.9.0. Just want to brainstorm around some ideas. There are also ways I could imagine this being done with a helper system on top of A-Frame...

AFRAME.registerSystem('async', {
  // ...
});

AFRAME.registerComponent('sword', {
  init: function () {
    var sceneEl = this.el.sceneEl;
    sceneEl.systems.async.awaitAll('mesh', 'body', 'material')
      .then(() => this.doStuff());
  },
  doStuff: function () {
    // ...
  }
});

The current API works well and it's possible to solve most any issue for a particular experience, but I think there could be things we could do (or document) to make it easier to write components that are more reliably reusable across many scenes.

ngokevin commented 6 years ago

I would be more comfortable doing this if lifecycle order were documented.

Suggested approach:

play: function () {
  if (this.hasSetupNavMesh) { return; }
  // ...
  this.hasSetupNavMesh = true;
}

I have also filed in the past to differentiate between scene initial play and a resume handler (Inspector).

Or better, if behavior depends on parent entities, the behavior should move to the parent entity or a manager.

About most of my components I develop within applications are application-agnostic and can be dropped into other apps. There's not much difference in building a component within an app and a robust reusable one.

donmccurdy commented 6 years ago

About most of my components I develop within applications are application-agnostic and can be dropped into other apps. There's not much difference in building a component within an app and a robust reusable one.

If by "robust" and "reusable" we mean (1) can be added at any point in the scene lifecycle, and (2) can be nested under parents with position/rotation/scale, then many components fall short of this. My own physics components and controls code, for starters. Both ngokevin/aframe-physics-components and kframe/components/geometry-merger make exactly the mistake I'm describing above, assuming that world position is available during init(), and will fail when nested under parents not located at the origin.

donmccurdy commented 6 years ago

Maybe the easy steps would be:

  1. Add a cute little diagram showing overall order of lifecycle methods and events.
  2. I'll experiment with async flow in my own components and report back if anything interesting comes of it.