search

aframevr / aframe-inspector

:mag: Visual inspector tool for A-Frame. Hit *<ctrl> + <alt> + i* on any A-Frame scene.
https://aframe.io/aframe-inspector/examples/
MIT License
655 stars 202 forks source link

Selecting a-scene crashes the React UI on aframe 1.6.0 and master #741

Closed vincentfretin closed 3 months ago

vincentfretin commented 3 months ago

image

It works on aframe 1.5.0

vincentfretin commented 3 months ago

Context of the error: props = {name: 'camera', schema: {…}, data: null, componentname: 'screenshot', isSingle: false, …} props.entity.getDOMAttribute(props.componentname) is undefined

image

on master:

document.querySelector('a-scene').getDOMAttribute('screenshot')
undefined
document.querySelector('a-scene').getAttribute('screenshot')
{width: 4096, height: 2048, camera: null}

on aframe 1.5.0

document.querySelector('a-scene').getDOMAttribute('screenshot')
{}

It's an easy fix, I can just rewrite the line to

entity.getDOMAttribute(props.componentname)?.[props.name]

@mrxz Do you confirm the change of behavior of getDOMAttribute returning undefined instead of {} was expected for a component set withsceneEl.setAttribute('screenshot', '') https://github.com/aframevr/aframe/blob/c590f85b30e0577442e4bf73c3f5fdedf1f21eab/src/core/scene/a-scene.js#L83 here with the latest changes aframe changes?

mrxz commented 3 months ago

@mrxz Do you confirm the change of behavior of getDOMAttribute returning undefined instead of {} was expected for a component set withsceneEl.setAttribute('screenshot', '') https://github.com/aframevr/aframe/blob/c590f85b30e0577442e4bf73c3f5fdedf1f21eab/src/core/scene/a-scene.js#L83 here with the latest changes aframe changes?

@vincentfretin Partially, one of the goals was to reduce memory usage, so the internal attrValue is only leased from the object pool when needed. As a result there are more situations where it will be undefined when accessed directly or through getDOMAttribute. In particular for object based single-property components and components with no/empty schemas.

I was, however, under the impression that the outward behaviour of getDOMAttribute hadn't changed for multi-property components. Mainly because this 8 year old unit test remained green, but I now see that the test has a bug as shallowDeepEqual(undefined, {}) surprisingly passes.

vincentfretin commented 2 months ago

We should at least fix the test to change the tested result then?