Skip to content

Conversation

@dmarcos
Copy link
Member

@dmarcos dmarcos commented Dec 18, 2016

The bug was discovered when doing:

el.setAttribute('geometry', {primitive: 'box'});
el.setAttribute('geometry', {primitive: 'sphere'});

After the first line the value of el.components.geometry.attrValue was {primitive: box}
After the update {"buffer":true,"primitive":"sphere","skipCache":false,"depth":1,"height":1,"width":1,"segmentsHeight":1,"segmentsWidth":1,"segmentsDepth":1} causing the default values of the box primitive to be applied to the sphere primitive after update.

Copy link
Member

@ngokevin ngokevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+wc


if (value && typeof value === 'object') {
return vecParseFloat(value);
coordinate = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Fill in missing values for each vector component.
return vecParseFloat({
  x: value.x || defaultVec & defaultVec.x,
  // ...
});
var el = this.el;
el.setAttribute('geometry', {primitive: 'sphere', radius: 10});
el.setAttribute('geometry', {primitive: 'box'});
assert.deepEqual(el.components.geometry.attrValue, {primitive: 'box', radius: 10});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should assert the component data as well with getAttribute.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I should probably move this to the setAttribute suite


test('the attribute cache only stores the modified properties', function () {
var el = this.el;
el.setAttribute('geometry', {primitive: 'sphere', radius: 10});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assert the attrValue before the change too for clarity. Perhaps start with a box.

@dmarcos dmarcos merged commit 0ed8ef6 into aframevr:master Dec 20, 2016
ngokevin pushed a commit to pulilab/aframe that referenced this pull request Feb 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

2 participants