React UI disappear when selecting a entity with texture on aframe 1.6.0 (works ok on aframe 1.5.0)

[vincentfretin]

Tested on the index.html from this repo, on aframe 1.5.0 no issue, on aframe 1.6.0, when selecting an entity with a texture, the cube or the floor, we get the stacktrace

Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
    at checkForNestedUpdates (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:27287:11)
    at scheduleUpdateOnFiber (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:25470:3)
    at Object.enqueueSetState (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:14067:7)
    at Component.setState (webpack-internal:///./node_modules/react/cjs/react.development.js:355:16)
    at TextureWidget.setValue (webpack-internal:///./src/components/widgets/TextureWidget.js:206:12)
    at TextureWidget.componentDidUpdate (webpack-internal:///./src/components/widgets/TextureWidget.js:145:14)
    at commitLayoutEffectOnFiber (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:23333:28)
    at commitLayoutMountEffects_complete (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:24683:9)
    at commitLayoutEffects_begin (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:24669:7)
    at commitLayoutEffects (webpack-internal:///./node_modules/react-dom/cjs/react-dom.development.js:24607:3)

and the React UI disappear.

in TextureWidget.js:206:12

    console.log(newValue, this.state.value);
    if (newValue && newValue !== this.state.value) {
      this.setValue(newValue);
    }

on aframe 1.6.0, it gives
<img id="crateImg" src="https://aframe.io/sample-assets/assets/images/wood/crate.gif" crossorigin="true">, '#crateImg'

on aframe 1.5.0, it gives
#crateImg, #crateImg

[vincentfretin]

on aframe 1.5.0
entity.components.material.attrValue gives
{src: '#crateImg'}

on aframe 1.6.0
it gives
{src: img#crateImg}
so the resolved selector
Double checking with your @mrxz to see if this new behavior is expected.
and how we can get the unresolved src selector here to fix the issue.

[vincentfretin]

aframe-inspector/src/components/components/PropertyRow.js

Line 117 in 9302f2a

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

is getting the value with
entity.getDOMAttribute('material').src so that's the resolved selector.

and in

aframe-inspector/src/components/widgets/TextureWidget.js

Line 97 in 9302f2a

var newValue = component.attrValue && component.attrValue[this.props.name];

it's getting the value in a different way, so that's already fishy. It should just take again this.props.value.

Even if I rewrite the react component to use

componentDidUpdate(prevProps) {
    // This will be triggered typically when the element is changed directly with
    // element.setAttribute.
    if (!Object.is(this.props.value, prevProps.value)) {
      this.setValue(this.props.value);
    }
  }

similar to the other widgets, I get an error when selecting a new texture from the modal and a different error from 1.6.0 and master, but that may be a different issue.

[mrxz]

Double checking with your @mrxz to see if this new behavior is expected.
and how we can get the unresolved src selector here to fix the issue.

@vincentfretin This is expected, parsing of properties now happens earlier and is stored to avoid repeated parsing as that just waste cycles and often includes allocations. The selector and asset based types are tricky in that regard, as parsing is non-deterministic (selector might match different elements at a later point) and there isn't always a trivial stringification to go back.

For selector and selectorAll they are not stored parsed on .attrValue, meaning .attrValue and getDOMAttribute() should both return the raw selector string. For assets, only id based elements are allowed, so it's possible to construct the string value through "#" + attrValue.id, though probably best to utilise the stringify method on the schema for this.
(As an aside, .attrValue is strictly speaking internal and should not be relied on, meaning getDOMAttribute() being the proper way to access its value, though in practice there is no real difference at this point)

Either way, there is no guarantee that a string was used in the first place. It's perfectly valid to directly feed an HTMLImageElement into properties expecting a selector or map using .setAttribute. This also holds true for 1.5.0 and earlier. Not sure what the best approach for the inspector is when it encounters such a situation.

[vincentfretin]

Thanks @mrxz for the insights.

For selector and selectorAll they are not stored parsed on .attrValue, meaning .attrValue and getDOMAttribute() should both return the raw selector string.

This is not what I see here, selector type used on material src is giving the HTMLImageElement, not #crateImg

I see now that getDOMAttribute() is just component.attrValue[this.props.name] really.
The new componentDidUpdate I wrote above is fixing the error.
Now I need to investigate another error in ModalTextures.js when clicking on another texture.

[mrxz]

This is not what I see here, selector type used on material src is giving the HTMLImageElement, not #crateImg

That's because src isn't a selector type, but an asset type (map to be precise: src/shaders/standard.js#L50). You can only use id selectors with those (for example #crateImg works, but a-assets > img:first-child won't work), and these are stored parsed on .attrValue, but because of that limitation should be reversible to a string using their id.

Now I need to investigate another error in ModalTextures.js when clicking on another texture.

There is a bug in 1.6.0 (fixed on master: aframevr/aframe#5529) that can cause issues when changing textures. Depending on the state of the texture in Three, replacing the texture might fail or could otherwise cause issues when disposing. So I'd first try to debug the issue with A-Frame master to rule out the above bug.

[vincentfretin]

On aframe 1.6.0, the threejs error is indeed aframevr/aframe#5529 and I don't have it on master.
The other issue doesn't seem related to aframe-inspector code, all the code is correct as far as I can tell.
The issue is in aframe
I can reproduce it in the console, replacing crateImg to floorImg texture on the cube:
document.querySelector('.boxClass').setAttribute('material', {src: '#floorImg'})
gives

src-loader.js:154
HEAD http://localhost:3333/examples/[object%20HTMLImageElement] 404 (Not Found)

that's here https://github.com/aframevr/aframe/blob/9436d4b5c555cb924c3f1d5fdf977b5f0a7624e2/src/utils/src-loader.js#L154

[vincentfretin]

That's probably a regression from aframevr/aframe#5454

[vincentfretin]

or maybe not, your changes didn't touch the validateSrc function.

[mrxz]

Not sure, I wasn't able reproduce the issue in a plain A-Frame project. Placing a break point in updateProperties in component.js, reveals that after calling .setAttribute there are two updates:

• One receiving { "src": "#crateImg" } as expected
• One receiving the string "src: [object HTMLImageElement]; color: #F16745" originating from the MutationObserver

So something in the inspector probably triggers this second update.

[mrxz]

Oh... found it. It's related to the fact that the example scene has debug="true". This forces the flushToDOM logic on every update. The stringify method for assets is just the default one causing it to become [object HTMLImageElement]. Going to create a fix for it.

[vincentfretin]

@mrxz I just tested again my branch #723
and latest master, I shouldn't have to update the link in index.html, the cache should have expired by now, but just to be sure I tested with latest https://cdn.jsdelivr.net/gh/aframevr/aframe@3cad96337fa22be307c7408452336fa82e1181ca/dist/aframe-master.min.js that includes aframevr/aframe#5544 but I still see
src-loader.js:154 HEAD http://localhost:3333/examples/[object%20HTMLImageElement] 404 (Not Found)
when I select the cube with wood texture and change it to grass texture.
Am I missing something?

[mrxz]

@vincentfretin It seems that the master builds aren't being updated any more (Notice the last commit date here: https://github.com/aframevr/aframe/tree/master/dist). While the build bot still makes changes and commits, it doesn't actually update the files in dist/ (e.g. commit: aframevr/aframe@3cad963)

@dmarcos Any idea what is up with the build bot?

[dmarcos]

I was messing around with node versions and stuff and had to reboot the server and I might had screwed things up. Sorry. I’ll look into it

[vincentfretin]

I guess you played with the node version because of aframe-site, you should probably merge aframevr/aframe-site#539 so you can use a more recent version of node on the aws machine instead of using nvm install v6.

[vincentfretin]

@mrxz With aframe master build I copied over manually, it seems there is an edge case that is not handled. You can execute in the console
document.querySelector('.boxClass').setAttribute('material', 'src', '')
this is what is executed when you empty the src input on the right panel (I added console.log in the updateEntity function to verify that), gives
GET http://localhost:3333/examples/undefined 404 (Not Found)
and the cube is black (missing texture) instead of becoming white (the selected color)
This worked properly on aframe 1.5.0.

[mrxz]

@vincentfretin Found the bug, there's indeed an edge case when "unsetting" a value. These are now recorded as undefined on the internal attrValue, which behaves virtually identical to an empty string, except when stringifying through flushToDOM. I've created a PR to skip over undefined values when stringifying (aframevr/aframe#5549), as even prior to 1.6.0 it's possible to end up with undefined values (albeit considerable less common).

This does mean a small change in behaviour where flushToDOM no longer serializes empty values (e.g. material="src: "), but that seems like a clear improvement to me. There shouldn't be a difference between not setting a property, and setting it and subsequently "unsetting" it IMO.

The test coverage of flushToDOM and the debug component could use some work. Especially debug seems to only react to setAttribute changes, missing changes caused by things like mixin updates or removeAttribute on individual properties.