Closed kfarr closed 1 year ago
3 related issues on aframevr repo:
Here is some research on how getAttribute
works, since that appears to correctly reflect inspector edits:
https://github.com/aframevr/aframe/blob/96b6f46eebd17655f137a1efdc86594040ff03a4/src/core/a-entity.js#L728
It defaults to object3D values for position, rotation, scale, visible instead of DOM values.
Perhaps we could do the same right here? Either copy the logic from getAttribute
or somehow call it. Might be easier to just replicate the logic since it's only those 4 cases of position,rotation,scale,visible.
So perhaps this is actually a pull request on aframevr repo instead?
@3DStreet sponsored me to work on this issue. Thank you! https://github.com/c-frame/sponsorship/issues/3
Duplicate entity calls entity.flushToDOM() (https://github.com/aframevr/aframe/blob/v1.4.0/src/core/a-entity.js#L707) that calls for each component component.flushToDOM() (https://github.com/aframevr/aframe/blob/v1.4.0/src/core/component.js#L268), it gets the value from this.attrValue that is the parsed version of what was in the DOM initially or the latest setAttribute call. Here it resets the value from the DOM because this attrValue didn't change, no setAttribute was called when moving via TransformControls from the viewport, the object3D.position is modified directly. For position, scale, rotation, that's indeed wrong, the latest value is from object3D.position/rotation/scale (this is what is modified when you move the entity from the viewport). So for those cases, instead of using attrValue, we would indeed think that we need to have a similar implementation as getAttribute https://github.com/aframevr/aframe/blob/96b6f46eebd17655f137a1efdc86594040ff03a4/src/core/a-entity.js#L731-L734 But if we do a fix in component.flushToDOM(), that would only works if you had the component initially on the entity. For example for scale, if you didn't have it, the entity.flushToDOM() wouldn't auto add the scale component, and so it wouldn't be present in the components of the duplicated entity either. I don't think that auto creating components from entity.flushToDOM() is a valid fix here.
As mentionned in https://github.com/aframevr/aframe/issues/4084, when you change the position from the right panel and then duplicate the entity, the position is correct. That's because the Vec3Widget onChange actually do a entity.setAttribute('position', {x:1, y:2, z:3}) so it's creating the position component or updating it so this.attrValue is updated.
The correct fix here is really to call setAttribute when editing via the TransformControls.
TransformControls is emitting the objectChange
event that is reemitted with a different entityupdate
event here https://github.com/aframevr/aframe-inspector/blob/72fd46159801ad37a14e87448611607b5d09a36e/src/lib/viewport.js#L61-L84
We currently listen on the entityupdate event in CommonComponents.js to force a react update and the right panel is getting directly the value from object3D.position, also for rotation doing the rad2deg conversion. https://github.com/aframevr/aframe-inspector/blob/72fd46159801ad37a14e87448611607b5d09a36e/src/components/components/CommonComponents.js#L58-L67
I propose to change the code there to do the setAttribute in the entityupdate
listener.
I believe #695 fixes the issue in the correct way. For #689 about mixin and gltf-model, that seems to be a completely different issue not related to this one, I didn't start to investigate it yet.
Your linked issue https://github.com/aframevr/aframe/issues/2823 seems to be more related to #689 and not this one.
I moved the fix out of the React component, a proper place is the entityupdate
listener in viewport.js
that also handle other things. That way the fix is still there if I'm doing a different UI in aframe-editor later. And I simplified it, just using detail.value
, we already have the rotation in degrees there.
This is the second of 2 errors encountered using the duplicate (clone) entity feature (keyboard shortcut
d
)cloneNode
to clone an entity.d
to duplicate, the entity will return to its prior location before making the changes in the editor.here is the flushtodom for an entity: https://github.com/aframevr/aframe/blob/v1.4.0/src/core/a-entity.js#L698
for each component in the entity, it runs this flushtodom: https://github.com/aframevr/aframe/blob/v1.4.0/src/core/component.js#L267
note how it is using
this.attrValue
instead ofgetAttribute([componentName], [attributeName])
which may be returning the values in the original html instead of the current values using A-Frame's recommended apihere is a video https://github.com/aframevr/aframe-inspector/assets/470477/b7cb3adb-1408-4709-9cd2-3a65b4990ca4
here is the URL to reproduce: https://mixin-gltf-url-error.glitch.me/
here are instructions to reproduce: press ctl-alt-i to invoke the inspector, change the position of the mixin firetruck, while the truck entity is still selected press
d
, note how the firetruck returns to its original positionexpected behavior: