Allow components to define before/after constraints #5478
Conversation
mrxz
force-pushed
the
update-order
branch
2 times, most recently
from
4478a5e to
bfb9636
Compare
src/utils/order.js
Outdated
| var warn = debug('utils:order:warn'); | ||
|
|
||
| /** | ||
| * Derives an ordering from the elements with before and after constraints. |
There was a problem hiding this comment.
order no descriptive name. not sure it needs to be a separate utils file. we can just include this function in a-scene
There was a problem hiding this comment.
The a-scene.js file is already pretty lengthy. I'd rather not append this to it, especially given that the implementation details aren't relevant from the POV of the scene.
Definitely open for (file) name suggestions. I left out component as it can also be applied to systems down the line, but if it helps clarity now, it can be included in the name.
There was a problem hiding this comment.
900 LOC is not super big and purpose of utils is factor out code used from multiple files or exported. When understanding a code base having tons of files that don't know how to relate to one another a bigger problem than big files. I would just incorporate this code to a-scene
There was a problem hiding this comment.
Don't really agree, but ultimately a subjective point, so added it to a-scene.js.
src/core/scene/a-scene.js
Outdated
| this.componentOrder = solveOrder(components, this.componentOrder); | ||
| this.addEventListener('componentregistered', function () { | ||
| // Recompute order | ||
| self.componentOrder = solveOrder(components, self.componentOrder); |
There was a problem hiding this comment.
solveOrder not super descriptive. sortComponents?
There was a problem hiding this comment.
It's not sorting, the components object isn't mutated in the process. It just returns an order which it arrives at by solving the set of before/after constraints, hence solveOrder.
Perhaps something like determineOrder (or determineComponentOrder) could work?
There was a problem hiding this comment.
sortComponentsByTikTokOrder ? It's not a sort in place but still a sort
There was a problem hiding this comment.
Not trying to be pedantic, but it really isn't sorting, neither in-place nor as its result. The resulting order could be used for sorting, but isn't. Naming it as such feels out of place to me.
|
This one is pretty close |
|
Thanks so much for the patient and being accommodating! Great work! |
Description:
The scene calls all the tick and tock methods in the order they were added. This can cause problems when components use the output of other components. They either get called after, in which case they have up-to-date info, or before, in which case they have 1 frame stale data. The longer the chain is the more frames of 'latency' this might add. This is often an issue with components that inspect or use the position of another element (e.g. collision detection before updated controller pose, nav-mesh constraints before locomotion, etc...). This can be seen in Ada's aframe-xr-boilerplate:
update-order-PR-vid.webm
This PR expand the component definition with
afterandbefore. A componentstickwill be called after alltickmethods of the components listed inafterhave taken place and before alltickmethods of the components listed inbefore. The scene updates per component, the order among components of the same type is still undefined.This PR also includes an alternative implementation of #5403 (fixes #5400, fixes #4164) . Since calls take place per component type, the odds that a component adds/removes components of the same type is unlikely, meaning that a happy path can be taken most of the time (directly add or remove behaviour). In case it does happen, the behaviours are marked for removal and removed after the scene is done with that particular component.
Changes proposed:
tickandtockper component respecting the given constraintsremoveBehaviorwhen called from atickortockmethod