Skip to content

Commit c9c8cf3

Browse files
dmarcosdmarcos
committed
Skip type checking if an object is passed through setAttribute twice
1 parent 0605ac9 commit c9c8cf3

File tree

8 files changed

+218
-52
lines changed

8 files changed

+218
-52
lines changed

‎examples/performance/cubes/components.js‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
/* global AFRAME, THREE */
2+
function randomIncRad (multiplier) {
3+
return multiplier * Math.random();
4+
}
5+
6+
function randomIncDeg (multiplier) {
7+
return randomIncRad(multiplier) * THREE.Math.RAD2DEG;
8+
}
9+
10+
// COMPONENTS
11+
// ------------------
12+
AFRAME.registerComponent('rotate-obj3d', {
13+
tick: function () {
14+
var el = this.el;
15+
el.object3D.rotation.x += randomIncRad(0.1);
16+
el.object3D.rotation.y += randomIncRad(0.2);
17+
el.object3D.rotation.z += randomIncRad(0.3);
18+
}
19+
});
20+
21+
AFRAME.registerComponent('rotate-get-obj3d', {
22+
tick: function () {
23+
var el = this.el;
24+
var rotation = el.getAttribute('rotation');
25+
26+
rotation.x += randomIncRad(0.1);
27+
rotation.y += randomIncRad(0.2);
28+
rotation.z += randomIncRad(0.3);
29+
30+
el.object3D.rotation.x = rotation.x;
31+
el.object3D.rotation.y = rotation.y;
32+
el.object3D.rotation.z = rotation.z;
33+
}
34+
});
35+
36+
AFRAME.registerComponent('rotate-obj3d-set', {
37+
tick: function () {
38+
var el = this.el;
39+
var rotation = el.getAttribute('rotation');
40+
41+
rotation.x += randomIncDeg(0.1);
42+
rotation.y += randomIncDeg(0.2);
43+
rotation.z += randomIncDeg(0.3);
44+
45+
el.setAttribute('rotation', rotation);
46+
}
47+
});
48+
49+
AFRAME.registerComponent('rotate-get-set', {
50+
tick: function () {
51+
var el = this.el;
52+
var rotationAux = this.rotationAux = this.rotationAux || {x: 0, y: 0, z: 0};
53+
var rotation = el.getAttribute('rotation');
54+
rotationAux.x = rotation.x + randomIncDeg(0.1);
55+
rotationAux.y = rotation.y + randomIncDeg(0.2);
56+
rotationAux.z = rotation.z + randomIncDeg(0.3);
57+
el.setAttribute('rotation', rotationAux);
58+
}
59+
});
60+
61+
AFRAME.registerComponent('rotate-get-settext', {
62+
tick: function () {
63+
var el = this.el;
64+
var rotation = el.getAttribute('rotation');
65+
66+
rotation.x += randomIncDeg(0.1);
67+
rotation.y += randomIncDeg(0.2);
68+
rotation.z += randomIncDeg(0.3);
69+
70+
el.setAttribute('rotation', rotation.x + ' ' + rotation.y + ' ' + rotation.z);
71+
}
72+
});

‎examples/performance/cubes/index.html‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<!DOCTYPE html>
2+
<html>
3+
<head>
4+
<title>Rotating Cubes Performance Test - A-Frame</title>
5+
<meta name="description" content="">
6+
<script src="../../../dist/aframe-master.js"></script>
7+
<script src="utils.js"></script>
8+
<script src="components.js"></script>
9+
</head>
10+
<style>
11+
body {
12+
color: #cccccc;
13+
font-family: Monospace;
14+
font-size: 13px;
15+
text-align: center;
16+
margin: 0px;
17+
overflow: hidden;
18+
}
19+
20+
#info {
21+
background-color: #000;
22+
position: absolute;
23+
top: 0px;
24+
width: 100%;
25+
padding: 5px;
26+
z-index: 9999;
27+
}
28+
#info a {
29+
color: #fff;
30+
}
31+
</style>
32+
<body>
33+
<div id="info">
34+
<a href="?numobjects=5000">none</a>
35+
<a href="?component=rotate-obj3d&numobjects=5000">rotate-obj3d</a>
36+
<a href="?component=rotate-get-obj3d&numobjects=5000">rotate-get-obj3d</a>
37+
<a href="?component=rotate-obj3d-set&numobjects=5000">rotate-obj3d-set</a>
38+
<a href="?component=rotate-get-set&numobjects=5000">rotate-get-set</a>
39+
<a href="?component=rotate-get-settext&numobjects=5000">rotate-get-settext</a>
40+
</div>
41+
<a-scene stats></a-scene>
42+
</body>
43+
<script src="setup.js"></script>
44+
</html>
45+

‎examples/performance/cubes/setup.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
var URL_PARAMS = getUrlParams();
2+
var DEFAULT_NUM_OBJECTS = 5000;
3+
var NUM_OBJECTS = URL_PARAMS.numobjects || DEFAULT_NUM_OBJECTS;
4+
var WIDTH = 100;
5+
var sceneEl = document.querySelector('a-scene');
6+
7+
for (var i = 0; i < NUM_OBJECTS; i++) {
8+
var entity = document.createElement('a-entity');
9+
10+
if (URL_PARAMS.component) {
11+
entity.setAttribute(URL_PARAMS.component, '');
12+
}
13+
14+
entity.setAttribute('position', getRandomPosition());
15+
entity.setAttribute('geometry', {primitive: 'box'});
16+
entity.setAttribute('material', {color: getRandomColor(), shader: 'flat'});
17+
sceneEl.appendChild(entity);
18+
}
19+
20+
function getRandomPosition () {
21+
return {
22+
x: Math.random() * WIDTH - WIDTH / 2,
23+
y: Math.random() * WIDTH - WIDTH / 2,
24+
z: Math.random() * WIDTH - WIDTH
25+
};
26+
}
27+
28+
function getRandomColor () {
29+
return '#' + ('000000' + Math.random().toString(16).slice(2, 8).toUpperCase()).slice(-6);
30+
}
31+
32+
function getUrlParams () {
33+
var match;
34+
var pl = /\+/g; // Regex for replacing addition symbol with a space
35+
var search = /([^&=]+)=?([^&]*)/g;
36+
var decode = function (s) { return decodeURIComponent(s.replace(pl, ' ')); };
37+
var query = window.location.search.substring(1);
38+
var urlParams = {};
39+
40+
match = search.exec(query);
41+
while (match) {
42+
urlParams[decode(match[1])] = decode(match[2]);
43+
match = search.exec(query);
44+
}
45+
return urlParams;
46+
}

‎src/core/a-entity.js‎

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -513,7 +513,7 @@ var proto = Object.create(ANode.prototype, {
513513
* @param {object} attrValue - The value of the DOM attribute.
514514
*/
515515
updateComponent: {
516-
value: function (attr, attrValue) {
516+
value: function (attr, attrValue, clobber) {
517517
var component = this.components[attr];
518518
var isDefault = attr in this.defaultComponents;
519519
if (component) {
@@ -522,7 +522,7 @@ var proto = Object.create(ANode.prototype, {
522522
return;
523523
}
524524
// Component already initialized. Update component.
525-
component.updateProperties(attrValue);
525+
component.updateProperties(attrValue, clobber);
526526
return;
527527
}
528528
// Component not yet initialized. Initialize component.
@@ -682,18 +682,19 @@ var proto = Object.create(ANode.prototype, {
682682
*/
683683
setAttribute: {
684684
value: function (attrName, arg1, arg2) {
685+
var arg1Type = typeof arg1;
686+
var clobber;
685687
var componentName;
686688
var isDebugMode;
687689

688690
// Determine which type of setAttribute to call based on the types of the arguments.
689691
componentName = attrName.split(MULTIPLE_COMPONENT_DELIMITER)[0];
690692
if (COMPONENTS[componentName]) {
691-
if (typeof arg1 === 'string' && typeof arg2 !== 'undefined') {
693+
if (arg1Type === 'string' && typeof arg2 !== 'undefined') {
692694
singlePropertyUpdate(this, attrName, arg1, arg2);
693-
} else if (typeof arg1 === 'object' && arg2 === true) {
694-
multiPropertyClobber(this, attrName, arg1);
695695
} else {
696-
componentUpdate(this, attrName, arg1);
696+
clobber = arg1Type !== 'object' || (arg1Type === 'object' && arg2 === true);
697+
this.updateComponent(attrName, arg1, clobber);
697698
}
698699

699700
// In debug mode, write component data up to the DOM.
@@ -712,30 +713,6 @@ var proto = Object.create(ANode.prototype, {
712713
el.updateComponentProperty(componentName, propName, propertyValue);
713714
}
714715

715-
/**
716-
* Just update multiple component properties at once for a multi-property component.
717-
* >> setAttribute('foo', {bar: 'baz'})
718-
*/
719-
function componentUpdate (el, componentName, propValue) {
720-
var component = el.components[componentName];
721-
if (component && typeof propValue === 'object') {
722-
// Extend existing component attribute value.
723-
el.updateComponent(
724-
componentName,
725-
utils.extendDeep(utils.extendDeep({}, component.attrValue), propValue));
726-
} else {
727-
el.updateComponent(componentName, propValue);
728-
}
729-
}
730-
731-
/**
732-
* Pass in complete data set for a multi-property component.
733-
* >> setAttribute('foo', {bar: 'baz'}, true)
734-
*/
735-
function multiPropertyClobber (el, componentName, propObject) {
736-
el.updateComponent(componentName, propObject);
737-
}
738-
739716
/**
740717
* Just update one of the component properties.
741718
* >> setAttribute('id', 'myEntity')

‎src/core/component.js‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -200,20 +200,28 @@ Component.prototype = {
200200
* @param {string} attrValue - HTML attribute value.
201201
* If undefined, use the cached attribute value and continue updating properties.
202202
*/
203-
updateProperties: function (attrValue) {
203+
updateProperties: function (attrValue, clobber) {
204204
var el = this.el;
205205
var isSinglePropSchema = isSingleProp(this.schema);
206206
var oldData = extendProperties({}, this.data, isSinglePropSchema);
207+
var skipTypeChecking = typeof this.previousAttrValue === 'object' && attrValue === this.previousAttrValue;
208+
var currentAttrValue = !clobber && this.attrValue;
209+
this.previousAttrValue = attrValue;
207210

208-
if (attrValue !== undefined) { this.updateCachedAttrValue(attrValue); }
211+
if (!el.hasLoaded) {
212+
if (attrValue !== undefined) { this.updateCachedAttrValue(attrValue); }
213+
return;
214+
}
209215

210-
if (!el.hasLoaded) { return; }
216+
attrValue = this.parseAttrValueForCache(attrValue);
211217

212218
if (this.updateSchema) {
213-
this.updateSchema(buildData(el, this.name, this.attrName, this.schema, this.attrValue,
219+
this.updateSchema(buildData(el, this.name, this.attrName, this.schema, attrValue, currentAttrValue,
214220
true));
215221
}
216-
this.data = buildData(el, this.name, this.attrName, this.schema, this.attrValue);
222+
this.data = buildData(el, this.name, this.attrName, this.schema, attrValue, currentAttrValue, false, skipTypeChecking);
223+
224+
if (attrValue !== undefined) { this.updateCachedAttrValue(attrValue); }
217225

218226
if (!this.initialized) {
219227
// Component is being already initialized
@@ -236,7 +244,7 @@ Component.prototype = {
236244
}, false);
237245
} else {
238246
// Don't update if properties haven't changed
239-
if (utils.deepEqual(oldData, this.data)) { return; }
247+
if (!skipTypeChecking && utils.deepEqual(oldData, this.data)) { return; }
240248

241249
// Update component.
242250
this.update(oldData);
@@ -390,7 +398,7 @@ module.exports.registerComponent = function (name, definition) {
390398
* @param {boolean} silent - Suppress warning messages.
391399
* @return {object} The component data
392400
*/
393-
function buildData (el, name, attrName, schema, elData, silent) {
401+
function buildData (el, name, attrName, schema, elData, oldElData, silent, skipTypeChecking) {
394402
var componentDefined = elData !== undefined && elData !== null;
395403
var data;
396404
var isSinglePropSchema = isSingleProp(schema);
@@ -400,9 +408,10 @@ function buildData (el, name, attrName, schema, elData, silent) {
400408
if (isSinglePropSchema) {
401409
data = schema.default;
402410
} else {
403-
data = {};
411+
data = typeof oldElData === 'object' ? utils.extend({}, oldElData) : {};
404412
Object.keys(schema).forEach(function applyDefault (key) {
405413
var defaultValue = schema[key].default;
414+
if (data[key]) { return; }
406415
data[key] = defaultValue && defaultValue.constructor === Object
407416
? utils.extend({}, defaultValue)
408417
: defaultValue;
@@ -420,14 +429,19 @@ function buildData (el, name, attrName, schema, elData, silent) {
420429

421430
// 3. Attribute values (highest precendence).
422431
if (componentDefined) {
423-
if (isSinglePropSchema) { return parseProperty(elData, schema); }
432+
if (isSinglePropSchema) {
433+
if (skipTypeChecking === true) { return elData; }
434+
return parseProperty(elData, schema);
435+
}
424436
data = extendProperties(data, elData, isSinglePropSchema);
425-
return parseProperties(data, schema, undefined, name, silent);
426437
} else {
427-
// Parse and coerce using the schema.
438+
if (skipTypeChecking === true) { return data; }
439+
// Parse and coerce using the schema.
428440
if (isSinglePropSchema) { return parseProperty(data, schema); }
429-
return parseProperties(data, schema, undefined, name, silent);
430441
}
442+
443+
if (skipTypeChecking === true) { return data; }
444+
return parseProperties(data, schema, undefined, name, silent);
431445
}
432446
module.exports.buildData = buildData;
433447

‎tests/components/scene/fog.test.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,10 +48,13 @@ suite('fog', function () {
4848

4949
test('can update fog type', function () {
5050
var el = this.el;
51-
el.setAttribute('fog', 'type: exponential; density: 0.25');
52-
el.setAttribute('fog', 'density: 0.25');
51+
assert.equal(el.getAttribute('fog').type, 'linear');
5352
assert.notOk('density' in el.object3D.fog);
5453
assert.ok('near' in el.object3D.fog);
54+
el.setAttribute('fog', 'type: exponential; density: 0.25');
55+
assert.equal(el.getAttribute('fog').type, 'exponential');
56+
assert.ok('density' in el.object3D.fog);
57+
assert.notOk('near' in el.object3D.fog);
5558
});
5659

5760
test('can remove and add linear fog', function () {

‎tests/core/a-entity.test.js‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -348,20 +348,17 @@ suite('a-entity', function () {
348348
assert.strictEqual(this.el.getAttribute('test').array, sourceArray);
349349
});
350350

351-
test('partial updates of array-type properties do not trigger update', function () {
351+
test('partial updates of array-type properties do trigger update', function () {
352352
// Updates to array do not trigger update handler.
353353
var updateSpy;
354354
registerComponent('test', {
355355
schema: {array: {type: 'array'}},
356356
update: function () { /* no-op */ }
357357
});
358-
this.el.setAttribute('test', {arr: [1, 2, 3]});
358+
this.el.setAttribute('test', {array: [1, 2, 3]});
359359
updateSpy = this.sinon.spy(this.el.components.test, 'update');
360-
// setAttribute does not trigger update because utils.extendDeep
361-
// called by componentUpdate assigns the new value directly into the
362-
// component data
363-
this.el.setAttribute('test', {arr: [4, 5, 6]});
364-
assert.isFalse(updateSpy.called);
360+
this.el.setAttribute('test', {array: [4, 5, 6]});
361+
assert.ok(updateSpy.called);
365362
});
366363

367364
test('can partially update vec3', function () {
@@ -1216,6 +1213,19 @@ suite('a-entity component lifecycle management', function () {
12161213
sinon.assert.calledOnce(TestComponent.update);
12171214
});
12181215

1216+
test('does not check types if the same object is passed again', function () {
1217+
var el = this.el;
1218+
var componentData;
1219+
var attrValue = {a: 3};
1220+
el.setAttribute('test', attrValue);
1221+
componentData = el.getAttribute('test');
1222+
assert.ok(componentData.a === 3);
1223+
attrValue.a = '3';
1224+
el.setAttribute('test', attrValue);
1225+
componentData = el.getAttribute('test');
1226+
assert.ok(componentData.a === '3');
1227+
});
1228+
12191229
test('calls remove on removeAttribute', function () {
12201230
var el = this.el;
12211231
var TestComponent = this.TestComponent.prototype;

‎tests/core/a-mixin.test.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ suite('a-mixin', function () {
3737
var mixinEl = document.createElement('a-mixin');
3838
el.setAttribute('mixin', 'ring');
3939
el.setAttribute('geometry', 'buffer: false');
40-
4140
mixinEl.setAttribute('id', 'ring');
4241
mixinEl.setAttribute('geometry', 'primitive: ring');
4342
this.assetsEl.appendChild(mixinEl);

0 commit comments

Comments
 (0)