Adjust iterator index when removing behaviors during tick/tock

[mrxz]

Description:
While looking into #5400 I noticed that following an element removal the tick method of the animation component wasn't called for that frame. The scene iterates over the tick/tock behaviours but doesn't handle the situation where the behaviour array is mutated mid iteration, which can result in tick/tock behaviours to be skipped.

While there are several possible solutions, letting the scene detect these cases and adjusting the loop indices seemed the most straightforward. It's worth noting that this bug is likely pretty common across A-Frame experiences, as it isn't tied to element removal per se, but any behaviour removal, meaning removing a component can trigger it. Luckily skipping the tick method for a frame isn't a problem for most components.

Changes proposed:

• Keep track of the iterator indices when iterating over the tick/tock behaviors
• When removing a behavior check if the iterator index needs to be adjusted

[dmarcos]

Expand variable name. iterator?

[mrxz]

It's so common to refer to iterators as it or iter to the point that iterator would harm readability IMHO. At the very least it would make the following for-loop too unwieldy, though it could as well become a while loop.

Let's first settle on the name for behaviorIterators as that would impact this local name as well.

[dmarcos]

I was not familiar with it. It might be in your context / environment.
We always avoid acronyms (only exception we have is el for element because of how pervasive it is). a while loop should be fine. Still not convinced with the iterator name that seems loaded and used in other contexts but need to think about an alternative. You're probably in a better mind space than me atm to come up with a better name

[dmarcos]

not sure if iterator is the best name here. looked at the code for 5 mins without context and hard to understand

[mrxz]

Open for suggestions. The name seemed quite fitting to me as an iterator over an array is effectively just an index and it makes it clear that it's relevant when we're iterating over the behaviours.

Perhaps behaviorCursors works better? It places a bit more emphasis on pointing to a specific element, which is also the invariant that should be upheld when adding or removing behaviours mid-iteration.

[dmarcos]

There's are JS Iterators so not sure

[dmarcos]

This took me a bit to understand. It's subtle so worried might be hard to maintain once the context is forgotten. An alternative. Instead of removing the behavior right away we flag it as needsRemove and then clean up at the end of the render loop. When things can change mid frame thinking there might be other stuff we want to postpone until the end?

[mrxz]

Instead of removing the behavior right away we flag it as needsRemove and then clean up at the end of the render loop

That was actually the first approach I had in mind, but it isn't without its drawbacks. The behaviour objects aren't really owned by the scene, so if it would flag the object itself with something like behaviors.tick[i].needsRemove, it would be writing into the component. I'd like to avoid that as the fewer reserved properties there are, the better IMO.

What also doesn't help is that behaviors could be added or removed during all stages of a frame (before, during or after any tick or tock from either a component or system). So not only will there be an additional post render operation performing the actual removal, both the tick and tock loops will need to skip over them and the addBehavior method will have to take it into account as well, probably removing the removal flag, since a behavior can be removed and added within the same frame before the actual removal loop runs.

It's that last point that actually made me not explore this option further as it would imply a behavioural change. As far as I know A-Frame makes no guarantees about the order in which ticks and tocks are called, but the current behaviour is essentially the order in which components are "added". While people shouldn't rely on it, I'm certain there are situations where people resolved inter-component dependencies by adjusting the order.

When things can change mid frame thinking there might be other stuff we want to postpone until the end?

The current ad-hoc changes aren't ideal, but postponing them until the end is going to have a cascading effect. Let's say we would want to guarantee that for any given component its tick method is always invoked before rendering. Currently this can be violated for the first frame of the component's existence when it's added from a system's tick method. Postponing the component initialization would work around this, but that pushes the question whether or not the DOM manipulations should also be postponed. Not to mention that any component chain would now take multiple frames, for example a component adds another component that emits an event that causes a system to create a new entity. This would then take 3 whole frames, whereas it now wraps up before the end of the frame.

I don't think changing it would be worth it, unless there are more (serious) bugs we're unaware of. But it's bound to change or break quite a few apps that relied on seeing changes reflected immediately.

It's relatively easy to workaround quirks related to frames where a component is added or repeatedly paused and resumed. These can have inconsistent number of invocations of tick and/or tock methods (0, 1 or more that frame), but will always be preceded by an init or play. The bug addressed in this PR is unpredictable and should be fixed one way or another.

[dmarcos]

More complex but wonder if it's worth moving the responsibility of calling the ticks from the scene back to the entity. That way we don't need to maintain a parallel structure with the ticks and can envision an API to control the order of component execution at the entity level. e.g:

<a-entity component-tick-order="hand-tracking-grab-controls, physics,..>`

I would add a tick function in a-entity called by a-scene that iterates over the components calling the tick methods if defined.

A bit of history. add/RemoveBehavior was added a bit preemptively thinking there might be other logic we might want to invoke every frame. So far that hasn't happened.

Thoughts?

[mrxz]

Moving this responsibility to the entity is going to make things harder, I'm afraid. Currently the "bookkeeping" burden only happens when adding or removing behaviours, meaning that the scene's tick and tock functions only perform what is needed. If instead it would have to traverse into the entities and the entities would need to check which tick/tock methods of its components to invoke, it's simply doing more unnecessary work every frame.

Not to mention that we'd still have to deal with the cases where things change mid-frame. Although instead of being concentrated on the resulting add/removeBehavior calls, it's now a combination of components and/or entities being added or removed.

(...) can envision an API to control the order of component execution at the entity level.

Component execution ordering would indeed be a very welcome addition. However, the order at the entity level is only part of the problem. Take for example a simple lookAt component. Ideally it's executed after any component that influences the location of the entity it's on (entity level) as well as after any component that influences the location of the target entity (cross entity).

And I'd rather avoid requiring users to specify the order manually, where possible. For the most part things should "just work", so tracked-controls for example should be one of the first components, so other components will guaranteed see the updated orientation instead of a stale one from last frame.

One approach is to order the registered components and execute the tick and tock methods following that order (like Unity allows). If components can express relative ordering ("before component Y" or "after component X") that would make it possible for the built-in components and any third party components to provide sane defaults. As an escape hatch we can allow users to override the ordering at the scene level and/or through code.

But let's not get too distracted. The way I see it there are three distinct "problems":

1. When components or entities are added (or resumed through play()), the invocations of tick and tock may or may not be called that same frame, or even multiple times (through repeated pause/play).
2. When components or entities are removed, random tick and/or tock methods can be skipped (Animation hiccups when an object is removed from the DOM #5400)
3. Ordering of the execution of components

This PR only address the second point. The first point is relatively easy to workaround for a user so doesn't need any action IMO. For the third point, perhaps best to open a dedicated issue to discuss it further and allow others to weigh in as well.

[dmarcos]

Solution I propose is simple. Scene just has to traverse the entities:

1. Scene keeps a list of the entities as they attach / remove themselves.
2. Scene traverses the entity list calling their tick method if it's a leaf entity (no children). This way we also preserve the tick order from children to parents (same as entity loading). Notice that now it's inconsistent.
3. Each entity calls the tick method of its components. Just have to check if the component is present before calling it. Don't think any additional work is required if things change mid-frame. Finally the entity calls the tick method of its parent.

We don't have to implement the component tick order at first pass just pointing out that the new structure makes it much easier to think about and implement an API.

[dmarcos]

Also for the play / pause issue. We could call the tick method of the entity when goes from paused to play so everything is up to date on the same frame. Harder to do with the current structure. We would have to make sure that the tick method is not invoked twice (one at play and other one by the scene tick). Out of children to parent tick execution of this might be ok but not sure yet. I see scenarios where only a tick or tock is invoked on the entity depending on when the entity is paused / played.

[dmarcos]

Lastly if a component is added we should probably call its tick method but not sure because could alter the children to parent execution order so might be more clear to just skip the current frame

[mrxz]

1. Scene keeps a list of the entities as they attach / remove themselves.

This still means that the scene will have to handle on-the-fly insertions and removals on a list while it iterates over it. Not that that is inherently bad, but we'll face the exact same situation as we do now for the entity list alone.

I think this is easier than I though especially if we follow a root to leaf tick order. sceneEl.children is the source of truth and copied over to a sceneEl.tickTockList at the beginning of each frame. can check for entity.isPlaying when iterating.

2. Scene traverses the entity list calling their tick method if it's a leaf entity (no children). This way we also preserve the tick order from children to parents (same as entity loading).

Entity loading is indeed from the leaves to the root, but the initial play() goes from root to leaves. And this currently determines the (initial) behaviour order. While I prefer a consistent order, I'm a bit hesitant turning it around as that is bound to break any app depending on the order.

And the entity list will be order sensitive, in that the parents have to precede their children. Otherwise it's possible for an entity to get its tick method called twice if it becomes a leaf node mid-frame.

Can be done from top to bottom too by calling root entities instead. Important is the consistency that also maintained as entities are added / removed.

3. Each entity calls the tick method of its components. Just have to check if the component is present before calling it. Don't think any additional work is required if things change mid-frame. Finally the entity calls the tick method of its parent.

Actually, it's this step that probably has to consider the most cases when it comes to mid-frame changes. While going over the components it's possible that components are added or removed. If the components are iterated using an index, we have the same situation as on the scene.

Won't use index. Iterate over the component object keys and check if the component still exists before calling tick

The real tricky part is when entities are added or removed. Entities added to leaf nodes that have been 'processed' already will become leaf nodes themselves. Either we don't process them, or we do, but then the child - parent order is broken for them. And if we do process them, extra care needs to be taken to prevent calling the tick of the parent again.

Removal of entities give similar problems. If the child entity is the last one (and thus responsible for calling the parent's tick method) and it gets removed, how is it going to handle this responsibility. Or worse, what if its parent is removed, who is going to call the tick of the grandparent?

played / added entities or components should not call tick / tock until next frame. I think makes everything more clear and predictable. also if tick executed from parent to leaves no issues with single child

Also for the play / pause issue. We could call the tick method of the entity when goes from paused to play so everything is up to date on the same frame. Harder to do with the current structure. We would have to just make sure that the tick method is not invoked twice (one at play and other one by the scene tick)

It'd be nice if there's only one mechanism that calls the tick methods. If the pause -> play transition could trigger a tick that also raises the question if it should do so when it happens during the systems' tick, during rendering or during the tock. It's probably safer to miss an "initial" tick than to have one out-of-order or during an unexpected "phase". In fact, we could even inhibit tick the frame an entity goes to play instead. While I don't really like the inconsistent this would create between new entities and resumed entities, at least we can guarantee this behaviour.

Lastly if a component is added we should probably call its tick method but not sure because could alter the children to parent execution order so might be more clear to just skip the current frame

Similar to the play/pause case above, the component might've missed the boat when it's added after the component tick handling. This will hold especially true when we support component execution ordering. Calling a tick out of order or during the wrong "phase" is probably worse than not calling it at all.

[dmarcos]

To sum up. Still think it's a better design for the scene to call tick on the root entities (parent is sceneEl).

1. Scene iterates over root entities (sceneEl.children) and calls tick method on them. We could avoid keeping track of the indices. At the beginning of a frame copy over this.children references into an auxiliary list and check if entities still exist / playing before calling tick. We're not allocating new memory and in practice we're talking about hundreds or low thousands of entities in a scene. I agree probably better to do parent to children. Important is consistency.

2. Each entity calls tick method on its children. Tick invocation logic is at a-entity level so shared with a-scene as well. For component ticks we can just iterate over the keys of the el.components object and check if still defined

3. when calling play on entity or component tick / tock won't call immediately but next frame. That prevents for example an entity added during the tock phase missing the tick method on that frame.

I think this makes tick / tock invocation more consistent and gives us a better playground to explore a component order API.

[dmarcos]

@mrxz What do you think of #5403 (comment)?

[dmarcos]

#5500 made this redundant?

[mrxz]

Yes, this is no longer needed.