aframevr / aframe

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

Setting position.x clobbers .y and .z #3120

Closed donmccurdy closed 6 years ago

donmccurdy commented 7 years ago

Open https://aframe.io/aframe/examples/boilerplate/hello-world/, and select the sphere element in Dev Tools. Then:

$0.setAttribute('position', {y: 2});
$0.flushToDOM();

Sphere's position changes from 0 1.25 -5 to 0 2 0. As of A-Frame 0.5.0 this behaved correctly, leaving .x and .z alone; I didn't check 0.6.0.

Issue filed here from comments from @MichaelHazani and others in Slack.

michaelybecker commented 7 years ago

Thanks @donmccurdy! I wonder if this is a threejs issue - gonna try to break it in three when I get home.

ngokevin commented 7 years ago

Perhaps turn them into multi prop components?

ngokevin commented 7 years ago

That would also allow for .setAttribute('position', 'x', 10) to not have to instantiate a JS object.

donmccurdy commented 7 years ago

Hm I'm not opposed to that change, but do you know why it's broken with single-prop components now when it worked in 0.5.0?

I'm assuming this applies to position and rotation components, and user-defined components too (like my velocity and quaternion).

ngokevin commented 7 years ago

Not sure. Before it probably worked by coincidence, but component code got refactored or changed, and now does not work. But making it multi-prop would make things easier, before if I wanted to update position, I'd have to getAttribute + change + clone (because getAttribute returns the data object) + setAttribute with another object.

dmarcos commented 7 years ago

Position, rotation, scale are really faux multiproperty components. They are single property components of the vec3 type (that has multiple attributes). We could make it a multiple property with a custom parser that would still allow for the form: el.setAttribute('position', '0 0 0');

machenmusik commented 7 years ago

(I thought those wanted to be THREE.Vector3 for performance efficiency)

ngokevin commented 7 years ago

You could still pass in a Vector3 if we move it to multi-prop.

ngokevin commented 6 years ago

A better way to update position now is el.object3D.position.y = 2. It will still be reflected in the A-Frame level.

donmccurdy commented 6 years ago

@ngokevin that's #3245 right? If we're fairly confident it will be a longterm feature, would be great to have some documentation of what to expect... is object3D.position the underlying storage for the position component? So object3D.position.x = 20 should not force any copying? It looks like getAttribute('position') returns a THREE.Vector3 now? But rotation is still an Object, since we need to convert from radians to degrees?

This feels like the right direction i think, just trying to make the API changes explicit.. seems possible this could break some components.

ngokevin commented 6 years ago

Docs will be added during the pre-release preparation, yeah. The explicit change is modifications to underlying object3D position/rotation/scale will be reflected at the A-Frame level through .getAttribute without any explicit syncing phase. It's a top highlight for me for 0.8.0 for ergonomics and likely performance.

ngokevin commented 6 years ago

object3D.position.y +=2; - shipped and docced