Vectors on schema with explicit properties are used by reference not by value.

[diarmidmackenzie]

Description:

• A-Frame Version: 1.3.0
• Platform / Device: All
• Reproducible Code Snippet or URL: https://glitch.com/edit/#!/vector-by-reference-not-value?path=index.html%3A38%3A9

Also: https://glitch.com/edit/#!/vec3-line-update-issue?path=index.html%3A25%3A53

Slightly complex to explain. Let's start with this code.

          vec = new THREE.Vector3(-1, 0, -4)
          box = document.getElementById("box1")
          box.setAttribute("position", vec)
          vec.set(1, 0, -4)
          box.setAttribute("position", vec)

Where does the box end up? At position 1, 0, 4 - as expected

But what about if we define a component like this:

      AFRAME.registerComponent("position-component", {
        schema: {
          position: { type: "vec3" }
        },
        update() {
          this.el.setAttribute("position", vec)
        }
      })

and use it in the exact same way...

          vec = new THREE.Vector3(-1, 0, -4)
          box = document.getElementById("box2")
          box.setAttribute("position-component", {position: vec})
          vec.set(1, 0, -4)
          box.setAttribute("position-component", {position: vec})

Now, the box ends up at position -1, 0, 4.... !

This happens for reasons described in this comment, which I'll explain again here..

The root cause is this code

function isObjectOrArray (value) {
  return value && (value.constructor === Object || value.constructor === Array) &&
         !(value instanceof window.HTMLElement);
}

this function isObjectOrArray is used to determine whether to copy or deep clone an object. It does not classify Vector3s as objects (because value.constructor is set to Vector3, not Object or Array)

This means that when A-Frame copies data to oldData, it just assigns the Vector3 objects by reference, rather than doing a deep copy.

In the case of the position attribute, which uses an unnamed single property in its schema, this doesn't happen. When we come to that code, the top-level vec3 object passed into setAttribute() has already been decomposed into x, y & z components.

But the problem does occur whenever a vec3 is used as a named property in a schema.

Examples of this causing problems with real components include:

• line component (start & end properties)
• raycaster component (origin & direction properties)

The effects manifest in a couple of ways in these different components.

The raycaster code uses data.origin and data.direction directly in code. As a result, if you write code like this:

vec3 = new Vector3(0, 0, 1)
el.setAttribute("raycaster", {origin: vec3}

then subsequent updates to vec3 result in immediate changes to the raycaster origin, and adjustments to the raycaster origin via setAttribute have no effect - the raycaster update() method doesn't even get called because A-Frame can't see any difference between the old data & the new data - they both refer to the same vec3.

One particular situation where this causes problems is trying to use multiple simultaneous cursors, as described here:
#5075 (comment)

For the line component it's a bit different. This doesn't directly use data.start and data.end, but copies the x, y & z data from these on in the update() method.

This means that with a line initialized like this...

vec3 = new Vector3(0, 0, 1)
el.setAttribute("line", {start: vec3}

then subequent updates to vec3 don't have any unexpected immediate effects on the line.

However, subsequent code like this will not update the line, as A-Frame won't even invoke the update() method for the same reasons as with raycaster

vec3.set(1, 1, 1)
el.setAttribute("line", {start: vec3})

See this additional glitch to illustrate the weirdness seen with updates to line start.
https://glitch.com/edit/#!/vec3-line-update-issue?path=index.html%3A25%3A53

I believe all this weirdness could be fixed by extending the checks here to consider Vector2, Vector3 & Vector4 as equivalent to arrays/objects and therefore needing a deep copy.

(I believe these are the only property types supported in the current schema API that would have this problem)

I have wondered whether there are significant (possibly intentional) performance benefits in re-using Vector3s, rather than cloning them, but I'm unconvinced that any derived performance benefits would be significant enough to justify all the counter-intuitive weirdness that arises by not cloning vectirs when they are passed on a component schema.

I'd be happy to take this further into a fix with some UTs (based on the glitches linked above) - but first would appreciate a review of the analysis above, and some agreement that vectors pass into components should be cloned (i.e. passed by value, not be reference).

[dmarcos]

That's intentional to avoid unnecessary allocations that can trigger garbage collection pauses. We should find a way to trigger the changes without cloning. We actually have an exception for position, scale, rotation where update is always triggered: https://github.com/aframevr/aframe/blob/master/src/core/component.js#L414

We can perhaps extend the exception to all vector type properties

[diarmidmackenzie]

As I see it there are 2 different issues here:
1 - changes to the vec3 object that was passed to the component are reflected immediately inside the component's data, without any call to setAttribute()
2 - some calls to setAttribute() do not trigger an update.

Forcing update to trigger would help with 2, but not with 1. While the glitch examples constructed focussed on issue 2, issue 1 is also problematic (e.g. has caused problems when using multiple cursors as they scribble over each others' raycaster vectors).

I wonder if we can get away with copying rather than cloning?

Outline proposal:

• when a vec3 appears on a component schema, allocate an additional vec3 for each instantiation of that component.
• when a vec3 is used in setAttribute, copy (not clone), the data into the private vec3 that was instantiated for that component.
  (same for vec2 & vec4)

This does result in additional vec3s being created & destroyed, but only at the rate of component/entity creation/destruction - which should be far slower than the rate at which component attributes can change (e.g. raycaster direction changes ~every frame).

I presume that creating/destroying entities & components already involves object allocations, hence it would be considered acceptable to create/destroy an additional vec3 or two as part of that processing?

Does that sound reasonable?

[dmarcos]

Allocating vec3s is what we want to avoid. Increases drastically memory footprint and risk of GC pauses. Performance takes precedence. Err on best perf side by default.

Issue 1 is a matter of documenting. If you want to retain state for vecX schemas the component should handle that internally. Maybe a flag could be offered for the vec4 schemas that copies the data automatically. Default should be no copy.

What I don't like it's inconsistency. Schemas with vecX will be updated on every setAttribute call. Shemas without won't. That can be confusing.

Another alternative is a special flag for setAttribute that forces update calls.

[diarmidmackenzie]

Yes, I understand we want to minimize allocation of vec3s, but there's a huge difference between allocating one per component instantiation, and allocating one per-update (which could be per-frame).

If we look at core A-Frame components that have this issue, we just have raycaster & line.

Raycasters you'll typically have a handful at most per-scene. A one-time allocation of a handful of vec3s at the point the scene is created can't create a significant perf issue.

With the line component, an application could conceivably create and destroy line components at a high rate of churn, potentially creating a GC issue. But for the line component, for each instance it's already allocating a THREE.LineBasicMaterial, a THREE.BufferGeometry, a THREE.BufferAttribute & a THREE.Line. A couple more vec3s on top of that is not going to make much of a difference.

Can you be more specific with the scenarios you are concerned about where allocating an additional vec3 for instance of a component with a vec3 on the schema is actually going to have a non-negligible impact on performance?

My personal experience is that the current behaviour manifests in really weird ways that take a huge amount of time to get to the bottom of. If I could save that time on my projects, and put it into more time performance tuning of my applications, I expect I'd easily make back the performance cost of those extra vec3s many times over.

[diarmidmackenzie]

A further thought - we have full control (via the component lifecycle) of when these vec3s are created & destroyed.

Could we mitigate the GC impact by having a pool of these vectors, allocating from that, and freeing back to it when done. Seems all the code to handle this logic already exists and is in use for a range of object types...?