supermedium / aframe-react

:atom: Build virtual reality experiences with A-Frame and React.
https://ngokevin.github.io/aframe-react-boilerplate/
MIT License
1.42k stars 151 forks source link

Primitives turn into geometry={{primitive:box}} when updating any geometry attribute with react state #57

Closed Stefie closed 7 years ago

Stefie commented 7 years ago

Hi, I'm working on a small aframe-react app where I need to update different geometry attributes via react state (like this: geometry={{ width: this.state.width }}).

When I'm using the geometry.primitive property everything works as expected, but when I'm using Primitives or the aframe-react-components module, all aframe-primitives are getting updated to geometry={{ primitive: 'box' }} when I update the react state.

This works as expected:

<Entity
  geometry={{ primitive: 'plane', width: this.state.width, height: '2' }}
  material={{color: 'blue' }}
  position='-4 -1.5 -8'
  onClick={this.changeWidth.bind(this)} />

This works onload, but when I update this.state.width the geometry.primitive of the <a-plane />-Element changes to 'box'

<Entity
   primitive='a-plane'
   geometry={{ width: this.state.width, height: '2' }}
   material={{color: 'blue' }}
   position='-4 2.5 -8'
   onClick={this.changeWidth.bind(this)} />

I used the aframe-react-boilerplate to build a basic demo of the bug. The upper row uses aframe Primitives, the lower row uses the geometry.primitive property. Live Demo with v0.4.0 (Code)

onload after this.setState({})
before-state-update after-state-update
Stefie commented 7 years ago

Hi, I kept thinking about this Bug so I did some tests: Every <Entity /> declared like this <Entity primitive='a-primitive' ... /> looses it's geometry-schema when a geometry property gets updated via react and they fall back to the aframe default-primitive 'box' declared in /aframe-master/src/components/geometry.js. When I change aframe's default primitive to 'sphere', they all turn into spheres ...I hope that helps a little.

ngokevin commented 7 years ago

Thanks, I might get around to patching this tonight.

Stefie commented 7 years ago

Thanks, that would be great! I'm building a little helper-app for the curvedimage which automatically calculates (und updates) the theta-length, height and/or radius based on the pixel values of an asset/image and the image ratio, so it would be great if I could use the a-curvedimage primitive for the preview.

ngokevin commented 7 years ago

Reproducing in a test case for v4.0.x, actively debugging it now.

ngokevin commented 7 years ago

Specifically, getting some weird issues on the setAttributes:

LOG: '%ccore:schema:warn %cUnknown property `0` for component/system `geometry`.%c +237ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `1` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `2` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `3` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `4` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `5` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `6` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `7` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `8` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `9` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `10` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `11` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `12` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `13` for component/system `geometry`.%c +0ms', 'color: orange', 'color: inherit', 'color: orange'
LOG: '%ccore:schema:warn %cUnknown property `14` for component/system `geometry`.%c +1ms', 'color: orange', 'color: inherit', 'color: orange
Stefie commented 7 years ago

I got this error a few times when I tried to use a non-existing geometry property. I think it's because aframe is looking for key/value pairs and if there's only a string without a colon to separate the key (property) from the value, it turns the string into an array and just iterates through each character of the string trying to find an attribute it recognizes.

If you do something like <Entity geometry="test" />, you're getting the error four times because the string test has four characters.

The string 'color: orange' is 15 characters long and you're getting the warning 15 times, so maybe aframe is trying to set the string 'color: orange'as a geometry property at some point without recognizing the colon : as a separator? (otherwise you would get LOG: '%ccore:schema:warn %cUnknown property 'color' for component/system 'geometry'...)

It's just a blind guess, though... but if you're getting the error 12 times with 'color: red' it could be the problem

robinnorth commented 7 years ago

@Stefie @ngokevin I am also getting the same log warnings on setAttribute calls after upgrading to aframe-react v4.0.2. This issue only occurs when passing component values as objects, and not strings. This leads me to believe that something is going wrong with stringifying these objects at some point.

Prior to v4.0.x, aframe-react handled stringifying props before allowing React to render A-Frame entities. Now, however, it bypasses the DOM and calls A-Frame's <a-entity>.setAttribute directly when passing props. This method is supposed to accept objects for known components (see https://aframe.io/docs/0.5.0/core/entity.html#setattribute-attr-value-componentattrvalue), but I note that the string '[object Object]' is 15 characters long, which matches the number of errors @ngokevin and I are seeing for each 'malformed' component prop...

Stefie commented 7 years ago

@robinnorth ah, the string [object Object] makes way more sense than the color property!

robinnorth commented 7 years ago

So this issue was caused by the referenced commit in v4.0.1, as A-Frame expects all the initial entity attributes to be strings (as outside of React, you'd just mark your scene up in HTML or render it with JS template partials), but if you pass in props in object form from React, they're not.

To fix this, I guess aframe-react needs to revert to serializing props into attributes for the initial render, but then continuing to bypass the DOM when updating the scene and using <a-entity>.setAttribute directly. @ngokevin, what are your thoughts on this?

ngokevin commented 7 years ago

A-Frame setAttribute can handle objects. If you pass an object, at no point should it ever try to be serialized. I thought perhaps the entity might not be ready yet so it uses the vanilla HTML setAttribute. I tried waiting for them to initialize, but didn't change anything. Still poking around, but we don't need to revert to serialization. This should work in theory.

robinnorth commented 7 years ago

@ngokevin I've logged out the initial setAttribute call that happens for an entity that exhibits the problem and the issue occurs when componentUpdate extends the existing component value that's retrieved from the DOM, which is being set by Entity's render method. Somewhere in this process object-based props are being incorrectly stringified. I can try and debug this further tomorrow if that's helpful.

ngokevin commented 7 years ago

Thanks, I'm poking at it now. I arrived at the same thing. It seems the initial setAttribute call will be done when the entity is not ready. Thus, the DOM will stringify [Object object]. I am trying to fix by waiting for the entity to be mounted before the initial call.

ngokevin commented 7 years ago

I've found the issue is on the initial render where it wasn't going through .setAttribute and React was trying to handle the whole process.

   86     return React.createElement(
   87       elementName,
  88       Object.assign({ref: this.updateDOM}, props),
   89       props.children);

If I remove props, things work better.

ngokevin commented 7 years ago

Removed the props, and the test I had passes now.

robinnorth commented 7 years ago

Nice one, @ngokevin! Can confirm this resolves the issue for me too.