From 8ce5d96c114cb7330f5587ce93542231720bc027 Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Tue, 8 Aug 2017 19:59:28 -0700 Subject: [PATCH 1/6] object pooling in component core, optimize style parser to reuse object --- src/components/camera.js | 5 + src/components/geometry.js | 4 +- src/components/material.js | 14 +- src/core/component.js | 260 +++++++++++++++++++++--------- src/utils/object-pool.js | 2 +- src/utils/styleParser.js | 122 ++++++++++++-- tests/components/geometry.test.js | 1 + tests/core/a-entity.test.js | 43 +++-- tests/core/component.test.js | 33 ++-- tests/core/controls.test.js | 2 +- 10 files changed, 366 insertions(+), 120 deletions(-) diff --git a/src/components/camera.js b/src/components/camera.js index a32599ecb46..e097848c9c9 100644 --- a/src/components/camera.js +++ b/src/components/camera.js @@ -79,6 +79,11 @@ module.exports.Component = registerComponent('camera', { // Camera disabled. Set camera to another camera. system.disableSpectatorCamera(); } + + this.savedPose = { + position: el.getAttribute('position'), + rotation: el.getAttribute('rotation') + }; }, /** diff --git a/src/components/geometry.js b/src/components/geometry.js index 1d00d65e4b2..b6004846e5e 100644 --- a/src/components/geometry.js +++ b/src/components/geometry.js @@ -61,12 +61,10 @@ module.exports.Component = registerComponent('geometry', { /** * Update geometry component schema based on geometry type. - * - * @param {object} data - New data passed by Component. */ updateSchema: function (data) { + var currentGeometryType = this.oldData && this.oldData.primitive; var newGeometryType = data.primitive; - var currentGeometryType = this.data && this.data.primitive; var schema = geometries[newGeometryType] && geometries[newGeometryType].schema; // Geometry has no schema. diff --git a/src/components/material.js b/src/components/material.js index db6a717f515..d8ecff5dfea 100644 --- a/src/components/material.js +++ b/src/components/material.js @@ -53,10 +53,16 @@ module.exports.Component = registerComponent('material', { }, updateSchema: function (data) { - var newShader = data.shader; - var currentShader = this.data && this.data.shader; - var shader = newShader || currentShader; - var schema = shaders[shader] && shaders[shader].schema; + var currentShader; + var newShader; + var schema; + var shader; + + newShader = data && data.shader; + currentShader = this.oldData && this.oldData.shader; + shader = newShader || currentShader; + schema = shaders[shader] && shaders[shader].schema; + if (!schema) { error('Unknown shader schema ' + shader); } if (currentShader && newShader === currentShader) { return; } this.extendSchema(schema); diff --git a/src/core/component.js b/src/core/component.js index 3f503f91747..7337d9cfb24 100644 --- a/src/core/component.js +++ b/src/core/component.js @@ -16,6 +16,7 @@ var warn = utils.debug('core:component:warn'); var aframeScript = document.currentScript; var upperCaseRegExp = new RegExp('[A-Z]+'); +var objectPool = utils.objectPool.createPool(); /** * Component class definition. @@ -37,13 +38,22 @@ var Component = module.exports.Component = function (el, attrValue, id) { this.attrName = this.name + (id ? '__' + id : ''); this.evtDetail = {id: this.id, name: this.name}; this.initialized = false; + this.isSingleProperty = isSingleProp(this.schema); + this.isSinglePropertyObject = this.isSingleProperty && + isObject(parseProperty(undefined, this.schema)); + this.isObjectBased = !this.isSingleProperty || this.isSinglePropertyObject; this.el.components[this.attrName] = this; // Store component data from previous update call. - this.oldData = undefined; + this.attrValue = undefined; + this.nextData = this.isObjectBased ? objectPool.use() : undefined; + this.oldData = this.isObjectBased ? objectPool.use() : undefined; + this.previousOldData = this.isObjectBased ? objectPool.use() : undefined; + this.parsingAttrValue = this.isObjectBased ? objectPool.use() : undefined; + // Purely for deciding to skip type checking. + this.previousAttrValue = undefined; // Last value passed to updateProperties. - this.previousAttrValue = undefined; this.throttledEmitComponentChanged = utils.throttle(function emitChange () { el.emit('componentchanged', self.evtDetail, false); }, 200); @@ -123,7 +133,7 @@ Component.prototype = { */ parse: function (value, silent) { var schema = this.schema; - if (isSingleProp(schema)) { return parseProperty(value, schema); } + if (this.isSingleProperty) { return parseProperty(value, schema); } return parseProperties(styleParser.parse(value), schema, true, this.name, silent); }, @@ -139,7 +149,7 @@ Component.prototype = { stringify: function (data) { var schema = this.schema; if (typeof data === 'string') { return data; } - if (isSingleProp(schema)) { return stringifyProperty(data, schema); } + if (this.isSingleProperty) { return stringifyProperty(data, schema); } data = stringifyProperties(data, schema); return styleParser.stringify(data); }, @@ -150,29 +160,52 @@ Component.prototype = { * @param {string} value - New data. * @param {boolean } clobber - Whether to wipe out and replace previous data. */ - updateCachedAttrValue: function (value, clobber) { - var attrValue = this.parseAttrValueForCache(value); - var isSinglePropSchema = isSingleProp(this.schema); - var property; - if (value === undefined) { return; } - - // Merge new data with previous `attrValue` if updating and not clobbering. - if (!isSinglePropSchema && !clobber) { - this.attrValue = this.attrValue ? cloneData(this.attrValue) : {}; - for (property in attrValue) { - this.attrValue[property] = attrValue[property]; + updateCachedAttrValue: (function () { + var tempObject = {}; + + return function (value, clobber) { + var newAttrValue; + var property; + + if (value === undefined) { return; } + + // If null value is the new attribute value, make the attribute value falsy. + if (value === null) { + if (this.isObjectBased && this.attrValue) { + objectPool.recycle(this.attrValue); + } + this.attrValue = undefined; + return; } - return; - } - // If single-prop schema or clobber. - this.attrValue = attrValue; - }, + if (value instanceof Object) { + // If value is an object, copy it to our pooled newAttrValue object to use to update + // the attrValue. + objectPool.clear(tempObject); + newAttrValue = utils.extend(tempObject, value); + } else { + newAttrValue = this.parseAttrValueForCache(value); + } + + // Merge new data with previous `attrValue` if updating and not clobbering. + if (this.isObjectBased && !clobber && this.attrValue) { + for (property in this.attrValue) { + if (!(property in newAttrValue)) { + newAttrValue[property] = this.attrValue[property]; + } + } + } + + // Update attrValue. + if (this.isObjectBased && !this.attrValue) { this.attrValue = objectPool.use(); } + objectPool.clear(this.attrValue); + this.attrValue = extendProperties(this.attrValue, newAttrValue, this.isObjectBased); + }; + })(), /** - * Given an HTML attribute value parses the string - * based on the component schema. To avoid double parsings of - * strings into strings we store the original instead + * Given an HTML attribute value parses the string based on the component schema. + * To avoid double parsings of strings into strings we store the original instead * of the parsed one * * @param {string} value - HTML attribute value @@ -180,7 +213,7 @@ Component.prototype = { parseAttrValueForCache: function (value) { var parsedValue; if (typeof value !== 'string') { return value; } - if (isSingleProp(this.schema)) { + if (this.isSingleProperty) { parsedValue = this.schema.parse(value); /** * To avoid bogus double parsings. Cached values will be parsed when building @@ -191,7 +224,8 @@ Component.prototype = { if (typeof parsedValue === 'string') { parsedValue = value; } } else { // Parse using the style parser to avoid double parsing of individual properties. - parsedValue = styleParser.parse(value); + objectPool.clear(this.parsingAttrValue); + parsedValue = styleParser.parse(value, this.parsingAttrValue); } return parsedValue; }, @@ -206,7 +240,7 @@ Component.prototype = { var attrValue = isDefault ? this.data : this.attrValue; if (!attrValue) { return; } window.HTMLElement.prototype.setAttribute.call(this.el, this.attrName, - this.stringify(attrValue)); + this.stringify(attrValue)); }, /** @@ -218,11 +252,15 @@ Component.prototype = { */ updateProperties: function (attrValue, clobber) { var el = this.el; - var isSinglePropSchema; var key; + var initialOldData; + var isSinglePropSchema; var skipTypeChecking; +<<<<<<< HEAD var oldData = this.oldData; var hasComponentChanged; +======= +>>>>>>> object pooling in component core, optimize style parser to reuse object // Just cache the attribute if the entity has not loaded // Components are not initialized until the entity has loaded @@ -254,8 +292,21 @@ Component.prototype = { // Cache previously passed attribute to decide if we skip type checking. this.previousAttrValue = attrValue; +<<<<<<< HEAD // Cache current attrValue for future updates. Updates `this.attrValue`. attrValue = this.parseAttrValueForCache(attrValue); +======= + // Parse the attribute value. + attrValue = this.parseAttrValueForCache(attrValue); + + // Update previous attribute value to later decide if we skip type checking. + this.previousAttrValue = attrValue; + + if (this.updateSchema) { this.updateSchema(this.buildData(attrValue, false, true)); } + this.data = this.buildData(attrValue, clobber, false, skipTypeChecking); + + // Cache current attrValue for future updates. +>>>>>>> object pooling in component core, optimize style parser to reuse object this.updateCachedAttrValue(attrValue, clobber); if (this.updateSchema) { @@ -272,17 +323,21 @@ Component.prototype = { this.init(); this.initialized = true; delete el.initializingComponents[this.name]; + + // Store current data as previous data for future updates. + this.oldData = extendProperties(this.oldData, this.data, this.isObjectBased); + // For oldData, pass empty object to multiple-prop schemas or object single-prop schema. // Pass undefined to rest of types. - oldData = (!isSinglePropSchema || - typeof parseProperty(undefined, this.schema) === 'object') ? {} : undefined; - // Store current data as previous data for future updates. - this.oldData = extendProperties({}, this.data, isSinglePropSchema); - this.update(oldData); + initialOldData = this.isObjectBased ? objectPool.use() : undefined; + this.update(initialOldData); + if (this.isObjectBased) { objectPool.recycle(initialOldData); } + // Play the component if the entity is playing. if (el.isPlaying) { this.play(); } el.emit('componentinitialized', this.evtDetail, false); } else { +<<<<<<< HEAD hasComponentChanged = !utils.deepEqual(this.oldData, this.data); // Don't update if properties haven't changed. // Always update rotation, position, scale. @@ -294,6 +349,27 @@ Component.prototype = { // In the case of position, rotation, scale always update the component // but don't emit componentchanged event if component has not changed. if (!hasComponentChanged) { return; } +======= + // Don't update if properties haven't changed + if (utils.deepEqual(this.oldData, this.data)) { return; } + + // Store the previous old data before we calculate the new oldData. + if (this.previousOldData instanceof Object) { objectPool.clear(this.previousOldData); } + if (this.isObjectBased) { + copyData(this.oldData, this.previousOldData); + } else { + this.previousOldData = this.oldData; + } + + // Store current data as previous data for future updates. + // Reuse `this.oldData` object to try not to allocate another one. + if (this.oldData instanceof Object) { objectPool.clear(this.oldData); } + this.oldData = extendProperties(this.oldData, this.data, this.isObjectBased); + + // Update component. + this.update(this.previousOldData); + +>>>>>>> object pooling in component core, optimize style parser to reuse object this.throttledEmitComponentChanged(); } }, @@ -305,7 +381,7 @@ Component.prototype = { * @param {string} propertyName - Name of property to reset. */ resetProperty: function (propertyName) { - if (isSingleProp(this.schema)) { + if (!this.isObjectBased) { this.attrValue = undefined; } else { if (!(propertyName in this.attrValue)) { return; } @@ -324,16 +400,17 @@ Component.prototype = { * @param {object} schemaAddon - Schema chunk that extend base schema. */ extendSchema: function (schemaAddon) { + var extendedSchema; // Clone base schema. - var extendedSchema = utils.extend({}, components[this.name].schema); + extendedSchema = utils.extend(objectPool.use(), components[this.name].schema); // Extend base schema with new schema chunk. utils.extend(extendedSchema, schemaAddon); this.schema = processSchema(extendedSchema); - this.el.emit('schemachanged', {component: this.name}); + this.el.emit('schemachanged', this.evtDetail); }, /** - * Builds component data from the current state of the entity, ultimately + * Build component data from the current state of the entity, ultimately * updating this.data. * * If the component was detached completely, set data to null. @@ -348,19 +425,18 @@ Component.prototype = { * @param {object} newData - Element new data. * @param {boolean} clobber - The previous data is completely replaced by the new one. * @param {boolean} silent - Suppress warning messages. - * @param {boolean} skipTypeChecking - Skip type checking and cohercion. + * @param {boolean} skipTypeChecking - Skip type checking and coercion. * @return {object} The component data */ buildData: function (newData, clobber, silent, skipTypeChecking) { var componentDefined; var data; var defaultValue; - var keys; - var keysLength; + var key; var mixinData; + var nextData = this.nextData; var schema = this.schema; var i; - var isSinglePropSchema = isSingleProp(schema); var mixinEls = this.el.mixinEls; var previousData; @@ -369,46 +445,57 @@ Component.prototype = { ? newData.length : newData !== undefined && newData !== null; + if (this.isObjectBased) { objectPool.clear(this.nextData); } + // 1. Default values (lowest precendence). - if (isSinglePropSchema) { - // Clone default value if plain object so components don't share the same object - // that might be modified by the user. - data = isObjectOrArray(schema.default) ? utils.clone(schema.default) : schema.default; + if (this.isSingleProperty) { + if (this.isObjectBased) { + // If object-based single-prop, then copy over the data to our pooled object. + data = copyData(schema.default, nextData); + } else { + // If is plain single-prop, copy by value the default. + data = schema.default; + } } else { // Preserve previously set properties if clobber not enabled. previousData = !clobber && this.attrValue; - // Clone previous data to prevent sharing references with attrValue that might be - // modified by the user. - data = typeof previousData === 'object' ? cloneData(previousData) : {}; + + // Clone default value if object so components don't share object + data = previousData instanceof Object + ? copyData(previousData, nextData) + : nextData; // Apply defaults. - for (i = 0, keys = Object.keys(schema), keysLength = keys.length; i < keysLength; i++) { - defaultValue = schema[keys[i]].default; - if (data[keys[i]] !== undefined) { continue; } + for (key in schema) { + defaultValue = schema[key].default; + if (data[key] !== undefined) { continue; } // Clone default value if object so components don't share object - data[keys[i]] = isObjectOrArray(defaultValue) ? utils.clone(defaultValue) : defaultValue; + data[keys] = isObjectOrArray(defaultValue) ? utils.clone(defaultValue) : defaultValue; } } // 2. Mixin values. for (i = 0; i < mixinEls.length; i++) { mixinData = mixinEls[i].getAttribute(this.attrName); - if (mixinData) { - data = extendProperties(data, mixinData, isSinglePropSchema); - } + if (mixinData) { data = extendProperties(data, mixinData, this.isObjectBased); } } // 3. Attribute values (highest precendence). if (componentDefined) { - if (isSinglePropSchema) { - if (skipTypeChecking === true) { return newData; } - return parseProperty(newData, schema); + if (this.isSingleProperty) { + if (skipTypeChecking === true) { return attrValue; } + // If object-based, copy the value to not modify the original. + if (this.isObjectBased) { + copyData(attrValue, this.parsingAttrValue); + return parseProperty(this.parsingAttrValue, schema); + } + return parseProperty(attrValue, schema); } - data = extendProperties(data, newData, isSinglePropSchema); + data = extendProperties(data, attrValue, this.isObjectBased); } else { if (skipTypeChecking === true) { return data; } // Parse and coerce using the schema. - if (isSinglePropSchema) { return parseProperty(data, schema); } + if (this.isSingleProperty) { return parseProperty(data, schema); } } if (skipTypeChecking === true) { return data; } @@ -487,6 +574,7 @@ module.exports.registerComponent = function (name, definition) { NewComponent.prototype.system = systems && systems.systems[name]; NewComponent.prototype.play = wrapPlay(NewComponent.prototype.play); NewComponent.prototype.pause = wrapPause(NewComponent.prototype.pause); + NewComponent.prototype.remove = wrapRemove(NewComponent.prototype.remove); components[name] = { Component: NewComponent, @@ -495,7 +583,8 @@ module.exports.registerComponent = function (name, definition) { multiple: NewComponent.prototype.multiple, parse: NewComponent.prototype.parse, parseAttrValueForCache: NewComponent.prototype.parseAttrValueForCache, - schema: utils.extend(processSchema(NewComponent.prototype.schema, NewComponent.prototype.name)), + schema: utils.extend(processSchema(NewComponent.prototype.schema, + NewComponent.prototype.name)), stringify: NewComponent.prototype.stringify, type: NewComponent.prototype.type }; @@ -509,15 +598,16 @@ module.exports.registerComponent = function (name, definition) { * @param data - Component data to clone. * @returns Cloned data. */ -function cloneData (data) { - var clone = {}; +function copyData (sourceData, dest) { var parsedProperty; var key; - for (key in data) { - parsedProperty = data[key]; - clone[key] = isObjectOrArray(parsedProperty) ? utils.clone(parsedProperty) : parsedProperty; + for (key in sourceData) { + parsedProperty = sourceData[key]; + dest[key] = isObjectOrArray(parsedProperty) + ? utils.clone(parsedProperty) + : parsedProperty; } - return clone; + return dest; } /** @@ -525,9 +615,10 @@ function cloneData (data) { * * @param dest - Destination object or value. * @param source - Source object or value -* @param {boolean} isSinglePropSchema - Whether or not schema is only a single property. +* @param {boolean} isObjectBased - Whether values are objects. * @returns Overridden object or value. */ +<<<<<<< HEAD function extendProperties (dest, source, isSinglePropSchema) { if (isSinglePropSchema && (source === null || typeof source !== 'object')) { return source; } var copy = utils.extend(dest, source); @@ -537,6 +628,13 @@ function extendProperties (dest, source, isSinglePropSchema) { copy[key] = utils.clone(copy[key]); } return copy; +======= +function extendProperties (dest, source, isObjectBased) { + if (isObjectBased && source instanceof Object) { + return utils.extend(dest, source); + } + return source; +>>>>>>> object pooling in component core, optimize style parser to reuse object } /** @@ -547,10 +645,10 @@ function hasBehavior (component) { } /** - * Wrapper for user defined pause method + * Wrapper for defined pause method. * Pause component by removing tick behavior and calling user's pause method. * - * @param pauseMethod {function} - user defined pause method + * @param pauseMethod {function} */ function wrapPause (pauseMethod) { return function pause () { @@ -565,11 +663,10 @@ function wrapPause (pauseMethod) { } /** - * Wrapper for user defined play method + * Wrapper for defined play method. * Play component by adding tick behavior and calling user's play method. * - * @param playMethod {function} - user defined play method - * + * @param playMethod {function} */ function wrapPlay (playMethod) { return function play () { @@ -584,6 +681,25 @@ function wrapPlay (playMethod) { }; } +/** + * Wrapper for defined remove method. + * Clean up memory. + * + * @param removeMethod {function} - Defined remove method. + */ +function wrapRemove (removeMethod) { + return function remove () { + removeMethod.call(this); + objectPool.recycle(this.attrValue); + objectPool.recycle(this.oldData); + objectPool.recycle(this.parsingAttrValue); + }; +} + +function isObject (value) { + return value && value.constructor === Object; +} + function isObjectOrArray (value) { return value && (value.constructor === Object || value.constructor === Array); } diff --git a/src/utils/object-pool.js b/src/utils/object-pool.js index 1345f5a40d6..7a899c7876e 100644 --- a/src/utils/object-pool.js +++ b/src/utils/object-pool.js @@ -72,7 +72,7 @@ module.exports.createPool = function createPool (objectFactory) { function clearObject (obj) { var key; - if (!(obj.constructor === Object)) { return; } + if (!obj || obj.constructor !== Object)) { return; } for (key in obj) { obj[key] = undefined; } } module.exports.clearObject = clearObject; diff --git a/src/utils/styleParser.js b/src/utils/styleParser.js index 00353f0a0ab..10ada4c3dbc 100644 --- a/src/utils/styleParser.js +++ b/src/utils/styleParser.js @@ -1,16 +1,20 @@ -/* Utils for parsing style-like strings (e.g., "primitive: box; width: 5; height: 4.5"). */ -var styleParser = require('style-attr'); +/** + * Utils for parsing style-like strings (e.g., "primitive: box; width: 5; height: 4.5"). + * Some code adapted from `style-attr` (https://github.com/joshwnj/style-attr) + * by Josh Johnston (MIT License). + */ /** - * Deserializes style-like string into an object of properties. + * Deserialize style-like string into an object of properties. * * @param {string} value - HTML attribute value. + * @param {object} obj - Reused object for object pooling. * @returns {object} Property data. */ -module.exports.parse = function (value) { +module.exports.parse = function (value, obj) { var parsedData; if (typeof value !== 'string') { return value; } - parsedData = styleParser.parse(value); + parsedData = styleParse(value, obj); // The style parser returns an object { "" : "test"} when fed a string if (parsedData['']) { return value; } return transformKeysToCamelCase(parsedData); @@ -24,7 +28,7 @@ module.exports.parse = function (value) { */ module.exports.stringify = function (data) { if (typeof data === 'string') { return data; } - return styleParser.stringify(data); + return styleStringify(data); }; /** @@ -34,8 +38,7 @@ module.exports.stringify = function (data) { * @return {string} CamelCased string. */ function toCamelCase (str) { - return str.replace(/-([a-z])/g, camelCase); - function camelCase (g) { return g[1].toUpperCase(); } + return str.replace(/-([a-z])/g, upperCase); } module.exports.toCamelCase = toCamelCase; @@ -47,12 +50,101 @@ module.exports.toCamelCase = toCamelCase; * @return {object} The object with keys camelCased. */ function transformKeysToCamelCase (obj) { - var keys = Object.keys(obj); - var camelCaseObj = {}; - keys.forEach(function (key) { - var camelCaseKey = toCamelCase(key); - camelCaseObj[camelCaseKey] = obj[key]; - }); - return camelCaseObj; + var camelKey; + var key; + for (key in obj) { + camelKey = toCamelCase(key); + if (key === camelKey) { continue; } + obj[camelKey] = obj[key]; + delete obj[key]; + } + return obj; } module.exports.transformKeysToCamelCase = transformKeysToCamelCase; + +/** + * Split a string into chunks matching `: ` + */ +var getKeyValueChunks = (function () { + var chunks = []; + var hasUnclosedUrl = /url\([^)]+$/; + + return function getKeyValueChunks (raw) { + var chunk = ''; + var nextSplit; + var offset = 0; + var sep = ';'; + + chunks.length = 0; + + while (offset < raw.length) { + nextSplit = raw.indexOf(sep, offset); + if (nextSplit === -1) { nextSplit = raw.length; } + + chunk += raw.substring(offset, nextSplit); + + // data URIs can contain semicolons, so make sure we get the whole thing + if (hasUnclosedUrl.test(chunk)) { + chunk += ';'; + offset = nextSplit + 1; + continue; + } + + chunks.push(chunk.trim()); + chunk = ''; + offset = nextSplit + 1; + } + + return chunks; + }; +})(); + +/** + * Convert a style attribute string to an object. + * + * @param {object} str - Attribute string. + * @param {object} obj - Object to reuse as a base, else a new one will be allocated. + */ +function styleParse (str, obj) { + var chunks; + var i; + var item; + var pos; + var key; + var val; + + obj = obj || {}; + + chunks = getKeyValueChunks(str); + for (i = 0; i < chunks.length; i++) { + item = chunks[i]; + if (!item) { continue; } + // Split with `.indexOf` rather than `.split` because the value may also contain colons. + pos = item.indexOf(':'); + key = item.substr(0, pos).trim(); + val = item.substr(pos + 1).trim(); + obj[key] = val; + } + return obj; +} + +/** + * Convert an object into an attribute string + **/ +function styleStringify (obj) { + var key; + var keyCount = 0; + var i = 0; + var str = ''; + + for (key in obj) { keyCount++; } + + for (key in obj) { + str += (key + ': ' + obj[key]); + if (i < keyCount - 1) { str += '; '; } + i++; + } + return str; +} + +function upperCase (str) { return str[1].toUpperCase(); } diff --git a/tests/components/geometry.test.js b/tests/components/geometry.test.js index b5ee6e70c38..5913490c2fb 100644 --- a/tests/components/geometry.test.js +++ b/tests/components/geometry.test.js @@ -18,6 +18,7 @@ suite('geometry', function () { suite('update', function () { test('allows empty geometry', function () { this.el.setAttribute('geometry', ''); + this.el.setAttribute('geometry', ''); }); test('creates geometry', function () { diff --git a/tests/core/a-entity.test.js b/tests/core/a-entity.test.js index 393822340cd..ff931360115 100644 --- a/tests/core/a-entity.test.js +++ b/tests/core/a-entity.test.js @@ -451,8 +451,9 @@ suite('a-entity', function () { assert.shallowDeepEqual(el.components.geometry.attrValue, { depth: 20, height: 10, + primitive: 'box', width: 5 - }); + }, 'Second attrValue'); assert.equal(geometry.width, 5, 'Second setAttribute'); done(); }); @@ -465,8 +466,9 @@ suite('a-entity', function () { suite('flushToDOM', function () { test('updates DOM attributes', function () { - var materialStr = 'color:#F0F;metalness:0.75'; + var el = this.el; var material; + var materialStr = 'color: #F0F; metalness: 0.75'; el.setAttribute('material', materialStr); material = HTMLElement.prototype.getAttribute.call(el, 'material'); assert.equal(material, ''); @@ -476,8 +478,9 @@ suite('a-entity', function () { }); test('updates DOM attributes of a multiple component', function () { - var soundStr = 'src:url(mysoundfile.mp3);autoplay:true'; + var el = this.el; var soundAttrValue; + var soundStr = 'src: url(mysoundfile.mp3); autoplay: true'; el.setAttribute('sound__1', {'src': 'url(mysoundfile.mp3)', autoplay: true}); soundAttrValue = HTMLElement.prototype.getAttribute.call(el, 'sound__1'); assert.equal(soundAttrValue, ''); @@ -490,7 +493,7 @@ suite('a-entity', function () { var childEl = document.createElement('a-entity'); var childMaterialStr = 'color:pink'; var materialAttr; - var materialStr = 'color:#F0F;metalness:0.75'; + var materialStr = 'color: #F0F; metalness: 0.75'; childEl.addEventListener('loaded', function () { materialAttr = HTMLElement.prototype.getAttribute.call(el, 'material'); assert.equal(materialAttr, null); @@ -500,7 +503,7 @@ suite('a-entity', function () { childEl.setAttribute('material', childMaterialStr); el.flushToDOM(true); materialAttr = HTMLElement.prototype.getAttribute.call(el, 'material'); - assert.equal(materialAttr, 'color:#F0F;metalness:0.75'); + assert.equal(materialAttr, 'color: #F0F; metalness: 0.75'); materialAttr = HTMLElement.prototype.getAttribute.call(childEl, 'material'); assert.equal(childMaterialStr, 'color:pink'); done(); @@ -904,21 +907,36 @@ suite('a-entity', function () { assert.equal(el.getAttribute('material').color, '#FFF'); }); - test('can clear mixins', function () { + test('does not remove default component', function () { + var el = this.el; + assert.ok('position' in el.components); + el.removeAttribute('position'); + assert.shallowDeepEqual(el.getDOMAttribute('position'), {}); + assert.ok('position' in el.components); + }); + + test('does not remove mixed-in component', function () { + var el = this.el; mixinFactory('foo', {position: '1 2 3'}); mixinFactory('bar', {scale: '1 2 3'}); el.setAttribute('mixin', 'foo bar'); - assert.shallowDeepEqual(el.getAttribute('position'), {x: 1, y: 2, z: 3}); - assert.shallowDeepEqual(el.getAttribute('scale'), {x: 1, y: 2, z: 3}); + assert.shallowDeepEqual(el.getAttribute('position'), {x: 1, y: 2, z: 3}, + 'Position mixin'); + assert.shallowDeepEqual(el.getAttribute('scale'), {x: 1, y: 2, z: 3}, + 'Scale mixin'); el.removeAttribute('mixin'); - assert.shallowDeepEqual(el.getAttribute('position'), {x: 0, y: 0, z: 0}); - assert.shallowDeepEqual(el.getAttribute('scale'), {x: 1, y: 1, z: 1}); + assert.shallowDeepEqual(el.getAttribute('position'), {x: 0, y: 0, z: 0}, + 'Position without mixin'); + assert.shallowDeepEqual(el.getAttribute('scale'), {x: 1, y: 1, z: 1}, + 'Scale without mixin'); }); }); suite('initComponent', function () { test('initializes component', function () { + var el = this.el; el.initComponent('material', false, 'color: #F0F; transparent: true'); + el.initComponent('material', 'color: #F0F; transparent: true', false); assert.ok(el.components.material); }); @@ -931,7 +949,8 @@ suite('a-entity', function () { }); test('initializes dependency component and can set attribute', function () { - el.initComponent('material', undefined, true); + var el = this.el; + el.initComponent('material', '', true); assert.shallowDeepEqual(el.getAttribute('material'), {}); }); @@ -1047,7 +1066,7 @@ suite('a-entity', function () { this.sinon.stub(el, 'setAttribute', nativeSetAttribute); this.sinon.stub(el, 'getAttribute', nativeGetAttribute); el.setAttribute('material', materialAttribute); - el.initComponent('material', true); + el.initComponent('material', '', true); assert.equal(el.getAttribute('material'), materialAttribute); }); diff --git a/tests/core/component.test.js b/tests/core/component.test.js index 0606bdd58d3..43ffbbd35cb 100644 --- a/tests/core/component.test.js +++ b/tests/core/component.test.js @@ -411,11 +411,12 @@ suite('Component', function () { el.setAttribute('material', ''); }); - test('a selector property default is not cloned into data', function () { + test('does not clone selector property default into data', function () { + var el; registerComponent('dummy', { schema: {type: 'selector', default: document.body} }); - var el = document.createElement('a-entity'); + el = document.createElement('a-entity'); el.hasLoaded = true; el.setAttribute('dummy', 'head'); el.components.dummy.updateProperties(''); @@ -423,10 +424,11 @@ suite('Component', function () { }); test('clones plain object schema default into data', function () { + var el; registerComponent('dummy', { schema: {type: 'vec3', default: {x: 1, y: 1, z: 1}} }); - var el = document.createElement('a-entity'); + el = document.createElement('a-entity'); el.hasLoaded = true; el.setAttribute('dummy', '2 2 2'); el.components.dummy.updateProperties(''); @@ -438,7 +440,6 @@ suite('Component', function () { var attrValue; var data; var el; - registerComponent('dummy', { schema: { color: {default: 'blue'}, @@ -450,6 +451,12 @@ suite('Component', function () { el.hasLoaded = true; el.setAttribute('dummy', ''); assert.notOk(el.components.dummy.attrValue.el); + }); + + test('does not clone props from attrValue into data that are not plain objects', function () { + var attrValue; + var el; + var data; // Direction property preserved across updateProperties calls but cloned into a different // object. @@ -672,7 +679,7 @@ suite('Component', function () { var component = new TestComponent(el); var componentObj = {position: {x: 1, y: 2, z: 3}}; var componentString = component.stringify(componentObj); - assert.deepEqual(componentString, 'position:1 2 3'); + assert.equal(componentString, 'position: 1 2 3'); }); }); @@ -792,7 +799,9 @@ suite('Component', function () { }); test('emit componentchanged when update calls setAttribute', function (done) { - var TestComponent = registerComponent('dummy', { + var component; + var TestComponent; + TestComponent = registerComponent('dummy', { schema: {color: {default: 'red'}}, update: function () { this.el.setAttribute('dummy', 'color', 'blue'); } }); @@ -801,11 +810,11 @@ suite('Component', function () { assert.equal(this.el.getAttribute('dummy').color, 'blue'); done(); }); - var component = new TestComponent(this.el); + component = new TestComponent(this.el); assert.equal(component.data.color, 'blue'); }); - test('oldData is empty object on the first call when a single property component with an object as default initializes', function () { + test('makes oldData empty object on first call when single property component with an object as default initializes', function () { var updateStub = sinon.stub(); registerComponent('dummy', { schema: {type: 'vec3'}, @@ -817,7 +826,7 @@ suite('Component', function () { assert.deepEqual(updateStub.getCalls()[0].args[0], {}); }); - test('oldData is empty object on the first call when a multiple property component initializes', function () { + test('makes oldData empty object on first call when multiple property component initializes', function () { var updateStub = sinon.stub(); registerComponent('dummy', { schema: { @@ -866,7 +875,7 @@ suite('Component', function () { assert.deepEqual(el.components.dummy.data, {x: 2, y: 2, z: 2}); }); - test('oldData and data is properly passed on recursive calls to setAttribute', function () { + test('properly passes oldData and data properly on recursive calls to setAttribute', function () { var el = this.el; registerComponent('dummy', { schema: { @@ -900,7 +909,7 @@ suite('Component', function () { el.setAttribute('dummy', {color: 'blue'}); assert.equal(HTMLElement.prototype.getAttribute.call(el, 'dummy'), ''); el.components.dummy.flushToDOM(); - assert.equal(HTMLElement.prototype.getAttribute.call(el, 'dummy'), 'color:blue'); + assert.equal(HTMLElement.prototype.getAttribute.call(el, 'dummy'), 'color: blue'); }); test('init and update are not called for a not loaded entity', function () { @@ -918,7 +927,7 @@ suite('Component', function () { el.components.dummy.flushToDOM(); sinon.assert.notCalled(initStub); sinon.assert.notCalled(updateStub); - assert.equal(HTMLElement.prototype.getAttribute.call(el, 'dummy'), 'color:blue'); + assert.equal(HTMLElement.prototype.getAttribute.call(el, 'dummy'), 'color: blue'); }); }); diff --git a/tests/core/controls.test.js b/tests/core/controls.test.js index 59cac49a8c6..77d500db570 100644 --- a/tests/core/controls.test.js +++ b/tests/core/controls.test.js @@ -1,4 +1,4 @@ -/* global Event, assert, process, setup, suite, test */ +/* global AFRAME, Event, assert, process, setup, suite, test */ var helpers = require('../helpers'); var PI = Math.PI; From f262c6362fef54675b40640017c46a6e02de001e Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Tue, 3 Oct 2017 18:59:45 +0200 Subject: [PATCH 2/6] improve pooling by not using key delete, make a pool for each type of component --- src/core/component.js | 107 +++++++++++++++++++++------------------ src/core/schema.js | 2 +- src/utils/object-pool.js | 2 +- 3 files changed, 60 insertions(+), 51 deletions(-) diff --git a/src/core/component.js b/src/core/component.js index 7337d9cfb24..7680931f528 100644 --- a/src/core/component.js +++ b/src/core/component.js @@ -16,7 +16,9 @@ var warn = utils.debug('core:component:warn'); var aframeScript = document.currentScript; var upperCaseRegExp = new RegExp('[A-Z]+'); -var objectPool = utils.objectPool.createPool(); + +// Object pools by component, created upon registration. +var objectPools = {}; /** * Component class definition. @@ -43,13 +45,14 @@ var Component = module.exports.Component = function (el, attrValue, id) { isObject(parseProperty(undefined, this.schema)); this.isObjectBased = !this.isSingleProperty || this.isSinglePropertyObject; this.el.components[this.attrName] = this; + this.objectPool = objectPools[this.name]; // Store component data from previous update call. this.attrValue = undefined; - this.nextData = this.isObjectBased ? objectPool.use() : undefined; - this.oldData = this.isObjectBased ? objectPool.use() : undefined; - this.previousOldData = this.isObjectBased ? objectPool.use() : undefined; - this.parsingAttrValue = this.isObjectBased ? objectPool.use() : undefined; + this.nextData = this.isObjectBased ? this.objectPool.use() : undefined; + this.oldData = this.isObjectBased ? this.objectPool.use() : undefined; + this.previousOldData = this.isObjectBased ? this.objectPool.use() : undefined; + this.parsingAttrValue = this.isObjectBased ? this.objectPool.use() : undefined; // Purely for deciding to skip type checking. this.previousAttrValue = undefined; @@ -160,48 +163,48 @@ Component.prototype = { * @param {string} value - New data. * @param {boolean } clobber - Whether to wipe out and replace previous data. */ - updateCachedAttrValue: (function () { - var tempObject = {}; - - return function (value, clobber) { - var newAttrValue; - var property; + updateCachedAttrValue: function (value, clobber) { + var newAttrValue; + var tempObject; + var property; - if (value === undefined) { return; } + if (value === undefined) { return; } - // If null value is the new attribute value, make the attribute value falsy. - if (value === null) { - if (this.isObjectBased && this.attrValue) { - objectPool.recycle(this.attrValue); - } - this.attrValue = undefined; - return; + // If null value is the new attribute value, make the attribute value falsy. + if (value === null) { + if (this.isObjectBased && this.attrValue) { + this.objectPool.recycle(this.attrValue); } + this.attrValue = undefined; + return; + } - if (value instanceof Object) { - // If value is an object, copy it to our pooled newAttrValue object to use to update - // the attrValue. - objectPool.clear(tempObject); - newAttrValue = utils.extend(tempObject, value); - } else { - newAttrValue = this.parseAttrValueForCache(value); - } + if (value instanceof Object) { + // If value is an object, copy it to our pooled newAttrValue object to use to update + // the attrValue. + tempObject = this.objectPool.use(); + newAttrValue = utils.extend(tempObject, value); + } else { + newAttrValue = this.parseAttrValueForCache(value); + } - // Merge new data with previous `attrValue` if updating and not clobbering. - if (this.isObjectBased && !clobber && this.attrValue) { - for (property in this.attrValue) { - if (!(property in newAttrValue)) { - newAttrValue[property] = this.attrValue[property]; - } + // Merge new data with previous `attrValue` if updating and not clobbering. + if (this.isObjectBased && !clobber && this.attrValue) { + for (property in this.attrValue) { + if (!(property in newAttrValue)) { + newAttrValue[property] = this.attrValue[property]; } } + } - // Update attrValue. - if (this.isObjectBased && !this.attrValue) { this.attrValue = objectPool.use(); } - objectPool.clear(this.attrValue); - this.attrValue = extendProperties(this.attrValue, newAttrValue, this.isObjectBased); - }; - })(), + // Update attrValue. + if (this.isObjectBased && !this.attrValue) { + this.attrValue = this.objectPool.use(); + } + utils.objectPool.clearObject(this.attrValue); + this.attrValue = extendProperties(this.attrValue, newAttrValue, this.isObjectBased); + utils.objectPool.clearObject(tempObject); + }, /** * Given an HTML attribute value parses the string based on the component schema. @@ -224,7 +227,7 @@ Component.prototype = { if (typeof parsedValue === 'string') { parsedValue = value; } } else { // Parse using the style parser to avoid double parsing of individual properties. - objectPool.clear(this.parsingAttrValue); + utils.objectPool.clearObject(this.parsingAttrValue); parsedValue = styleParser.parse(value, this.parsingAttrValue); } return parsedValue; @@ -329,9 +332,9 @@ Component.prototype = { // For oldData, pass empty object to multiple-prop schemas or object single-prop schema. // Pass undefined to rest of types. - initialOldData = this.isObjectBased ? objectPool.use() : undefined; + initialOldData = this.isObjectBased ? this.objectPool.use() : undefined; this.update(initialOldData); - if (this.isObjectBased) { objectPool.recycle(initialOldData); } + if (this.isObjectBased) { this.objectPool.recycle(initialOldData); } // Play the component if the entity is playing. if (el.isPlaying) { this.play(); } @@ -354,7 +357,9 @@ Component.prototype = { if (utils.deepEqual(this.oldData, this.data)) { return; } // Store the previous old data before we calculate the new oldData. - if (this.previousOldData instanceof Object) { objectPool.clear(this.previousOldData); } + if (this.previousOldData instanceof Object) { + utils.objectPool.clearObject(this.previousOldData); + } if (this.isObjectBased) { copyData(this.oldData, this.previousOldData); } else { @@ -363,7 +368,7 @@ Component.prototype = { // Store current data as previous data for future updates. // Reuse `this.oldData` object to try not to allocate another one. - if (this.oldData instanceof Object) { objectPool.clear(this.oldData); } + if (this.oldData instanceof Object) { utils.objectPool.clearObject(this.oldData); } this.oldData = extendProperties(this.oldData, this.data, this.isObjectBased); // Update component. @@ -402,7 +407,7 @@ Component.prototype = { extendSchema: function (schemaAddon) { var extendedSchema; // Clone base schema. - extendedSchema = utils.extend(objectPool.use(), components[this.name].schema); + extendedSchema = utils.extend({}, components[this.name].schema); // Extend base schema with new schema chunk. utils.extend(extendedSchema, schemaAddon); this.schema = processSchema(extendedSchema); @@ -445,7 +450,7 @@ Component.prototype = { ? newData.length : newData !== undefined && newData !== null; - if (this.isObjectBased) { objectPool.clear(this.nextData); } + if (this.isObjectBased) { utils.objectPool.clearObject(nextData); } // 1. Default values (lowest precendence). if (this.isSingleProperty) { @@ -576,11 +581,15 @@ module.exports.registerComponent = function (name, definition) { NewComponent.prototype.pause = wrapPause(NewComponent.prototype.pause); NewComponent.prototype.remove = wrapRemove(NewComponent.prototype.remove); + // Create object pool for class of components. + objectPools[name] = utils.objectPool.createPool(); + components[name] = { Component: NewComponent, dependencies: NewComponent.prototype.dependencies, isSingleProp: isSingleProp(NewComponent.prototype.schema), multiple: NewComponent.prototype.multiple, + name: name, parse: NewComponent.prototype.parse, parseAttrValueForCache: NewComponent.prototype.parseAttrValueForCache, schema: utils.extend(processSchema(NewComponent.prototype.schema, @@ -690,9 +699,9 @@ function wrapPlay (playMethod) { function wrapRemove (removeMethod) { return function remove () { removeMethod.call(this); - objectPool.recycle(this.attrValue); - objectPool.recycle(this.oldData); - objectPool.recycle(this.parsingAttrValue); + this.objectPool.recycle(this.attrValue); + this.objectPool.recycle(this.oldData); + this.objectPool.recycle(this.parsingAttrValue); }; } diff --git a/src/core/schema.js b/src/core/schema.js index 2d6299b2fa4..4b3a4f3ae1a 100644 --- a/src/core/schema.js +++ b/src/core/schema.js @@ -128,7 +128,7 @@ module.exports.parseProperties = (function () { // Validation errors. for (propName in propData) { - if (!schema[propName] && !silent) { + if (propData[propName] !== undefined && !schema[propName] && !silent) { warn('Unknown property `' + propName + '` for component/system `' + componentName + '`.'); } diff --git a/src/utils/object-pool.js b/src/utils/object-pool.js index 7a899c7876e..9a2cb4008c7 100644 --- a/src/utils/object-pool.js +++ b/src/utils/object-pool.js @@ -72,7 +72,7 @@ module.exports.createPool = function createPool (objectFactory) { function clearObject (obj) { var key; - if (!obj || obj.constructor !== Object)) { return; } + if (!obj || obj.constructor !== Object) { return; } for (key in obj) { obj[key] = undefined; } } module.exports.clearObject = clearObject; From 6a9ee260ca320fa212e0ca2ce4f7e2c3fcd82735 Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Thu, 5 Oct 2017 23:32:36 +0200 Subject: [PATCH 3/6] fixes to component optimizations --- src/core/component.js | 40 +++++++++++++++++++++--------------- tests/core/component.test.js | 6 ++++-- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/src/core/component.js b/src/core/component.js index 7680931f528..6f4b126f709 100644 --- a/src/core/component.js +++ b/src/core/component.js @@ -191,7 +191,7 @@ Component.prototype = { // Merge new data with previous `attrValue` if updating and not clobbering. if (this.isObjectBased && !clobber && this.attrValue) { for (property in this.attrValue) { - if (!(property in newAttrValue)) { + if (newAttrValue[property] === undefined) { newAttrValue[property] = this.attrValue[property]; } } @@ -257,7 +257,6 @@ Component.prototype = { var el = this.el; var key; var initialOldData; - var isSinglePropSchema; var skipTypeChecking; <<<<<<< HEAD var oldData = this.oldData; @@ -272,8 +271,6 @@ Component.prototype = { return; } - isSinglePropSchema = isSingleProp(this.schema); - // Disable type checking if the passed attribute is an object and has not changed. skipTypeChecking = attrValue !== null && typeof this.previousAttrValue === 'object' && attrValue === this.previousAttrValue; @@ -361,7 +358,7 @@ Component.prototype = { utils.objectPool.clearObject(this.previousOldData); } if (this.isObjectBased) { - copyData(this.oldData, this.previousOldData); + copyData(this.previousOldData, this.oldData); } else { this.previousOldData = this.oldData; } @@ -456,10 +453,12 @@ Component.prototype = { if (this.isSingleProperty) { if (this.isObjectBased) { // If object-based single-prop, then copy over the data to our pooled object. - data = copyData(schema.default, nextData); + data = copyData(nextData, schema.default); } else { // If is plain single-prop, copy by value the default. - data = schema.default; + data = isObjectOrArray(schema.default) + ? utils.clone(schema.default) + : schema.default; } } else { // Preserve previously set properties if clobber not enabled. @@ -467,7 +466,7 @@ Component.prototype = { // Clone default value if object so components don't share object data = previousData instanceof Object - ? copyData(previousData, nextData) + ? copyData(nextData, previousData) : nextData; // Apply defaults. @@ -475,28 +474,31 @@ Component.prototype = { defaultValue = schema[key].default; if (data[key] !== undefined) { continue; } // Clone default value if object so components don't share object - data[keys] = isObjectOrArray(defaultValue) ? utils.clone(defaultValue) : defaultValue; + data[key] = isObjectOrArray(defaultValue) + ? utils.clone(defaultValue) + : defaultValue; } } // 2. Mixin values. for (i = 0; i < mixinEls.length; i++) { mixinData = mixinEls[i].getAttribute(this.attrName); - if (mixinData) { data = extendProperties(data, mixinData, this.isObjectBased); } + if (!mixinData) { continue; } + data = extendProperties(data, mixinData, this.isObjectBased); } // 3. Attribute values (highest precendence). if (componentDefined) { if (this.isSingleProperty) { - if (skipTypeChecking === true) { return attrValue; } + if (skipTypeChecking === true) { return newData; } // If object-based, copy the value to not modify the original. if (this.isObjectBased) { - copyData(attrValue, this.parsingAttrValue); + copyData(this.parsingAttrValue, newData); return parseProperty(this.parsingAttrValue, schema); } - return parseProperty(attrValue, schema); + return parseProperty(newData, schema); } - data = extendProperties(data, attrValue, this.isObjectBased); + data = extendProperties(data, newData, this.isObjectBased); } else { if (skipTypeChecking === true) { return data; } // Parse and coerce using the schema. @@ -607,10 +609,11 @@ module.exports.registerComponent = function (name, definition) { * @param data - Component data to clone. * @returns Cloned data. */ -function copyData (sourceData, dest) { +function copyData (dest, sourceData) { var parsedProperty; var key; for (key in sourceData) { + if (sourceData[key] === undefined) { continue; } parsedProperty = sourceData[key]; dest[key] = isObjectOrArray(parsedProperty) ? utils.clone(parsedProperty) @@ -639,8 +642,13 @@ function extendProperties (dest, source, isSinglePropSchema) { return copy; ======= function extendProperties (dest, source, isObjectBased) { + var key; if (isObjectBased && source instanceof Object) { - return utils.extend(dest, source); + for (key in source) { + if (source[key] === undefined) { continue; } + dest[key] = source[key]; + } + return dest; } return source; >>>>>>> object pooling in component core, optimize style parser to reuse object diff --git a/tests/core/component.test.js b/tests/core/component.test.js index 43ffbbd35cb..f98fdabf374 100644 --- a/tests/core/component.test.js +++ b/tests/core/component.test.js @@ -869,8 +869,10 @@ suite('Component', function () { direction.z += 1; el.setAttribute('dummy', direction); sinon.assert.calledTwice(updateStub); - // old Data passed to the update method - assert.deepEqual(updateStub.getCalls()[0].args[0], {}); + // oldData passed to the update method. + assert.equal(updateStub.getCalls()[0].args[0].x, undefined); + assert.equal(updateStub.getCalls()[0].args[0].y, undefined); + assert.equal(updateStub.getCalls()[0].args[0].z, undefined); assert.deepEqual(updateStub.getCalls()[1].args[0], {x: 1, y: 1, z: 1}); assert.deepEqual(el.components.dummy.data, {x: 2, y: 2, z: 2}); }); From ece2fe8f9e39e33b66e46ad083693646924ece16 Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Wed, 26 Sep 2018 02:29:33 -0700 Subject: [PATCH 4/6] continue rebase, fix tests --- src/core/component.js | 55 ++--- src/utils/styleParser.js | 3 +- tests/components/obj-model.test.js | 41 ++-- tests/core/a-entity.test.js | 14 -- tests/core/component.test.js | 94 +++++---- tests/core/controls.test.js | 2 +- tests/core/shader.test.js | 325 ++++++++++++++--------------- 7 files changed, 259 insertions(+), 275 deletions(-) diff --git a/src/core/component.js b/src/core/component.js index 6f4b126f709..58894805ea5 100644 --- a/src/core/component.js +++ b/src/core/component.js @@ -258,11 +258,7 @@ Component.prototype = { var key; var initialOldData; var skipTypeChecking; -<<<<<<< HEAD - var oldData = this.oldData; var hasComponentChanged; -======= ->>>>>>> object pooling in component core, optimize style parser to reuse object // Just cache the attribute if the entity has not loaded // Components are not initialized until the entity has loaded @@ -292,11 +288,8 @@ Component.prototype = { // Cache previously passed attribute to decide if we skip type checking. this.previousAttrValue = attrValue; -<<<<<<< HEAD - // Cache current attrValue for future updates. Updates `this.attrValue`. - attrValue = this.parseAttrValueForCache(attrValue); -======= // Parse the attribute value. + // Cache current attrValue for future updates. Updates `this.attrValue`. attrValue = this.parseAttrValueForCache(attrValue); // Update previous attribute value to later decide if we skip type checking. @@ -306,7 +299,6 @@ Component.prototype = { this.data = this.buildData(attrValue, clobber, false, skipTypeChecking); // Cache current attrValue for future updates. ->>>>>>> object pooling in component core, optimize style parser to reuse object this.updateCachedAttrValue(attrValue, clobber); if (this.updateSchema) { @@ -337,22 +329,6 @@ Component.prototype = { if (el.isPlaying) { this.play(); } el.emit('componentinitialized', this.evtDetail, false); } else { -<<<<<<< HEAD - hasComponentChanged = !utils.deepEqual(this.oldData, this.data); - // Don't update if properties haven't changed. - // Always update rotation, position, scale. - if (!this.isPositionRotationScale && !hasComponentChanged) { return; } - // Store current data as previous data for future updates. - this.oldData = extendProperties({}, this.data, isSinglePropSchema); - // Update component. - this.update(oldData); - // In the case of position, rotation, scale always update the component - // but don't emit componentchanged event if component has not changed. - if (!hasComponentChanged) { return; } -======= - // Don't update if properties haven't changed - if (utils.deepEqual(this.oldData, this.data)) { return; } - // Store the previous old data before we calculate the new oldData. if (this.previousOldData instanceof Object) { utils.objectPool.clearObject(this.previousOldData); @@ -363,15 +339,20 @@ Component.prototype = { this.previousOldData = this.oldData; } + hasComponentChanged = !utils.deepEqual(this.oldData, this.data); + + // Don't update if properties haven't changed. + // Always update rotation, position, scale. + if (!this.isPositionRotationScale && !hasComponentChanged) { return; } + // Store current data as previous data for future updates. // Reuse `this.oldData` object to try not to allocate another one. if (this.oldData instanceof Object) { utils.objectPool.clearObject(this.oldData); } this.oldData = extendProperties(this.oldData, this.data, this.isObjectBased); - // Update component. + // Update component with the previous old data. this.update(this.previousOldData); ->>>>>>> object pooling in component core, optimize style parser to reuse object this.throttledEmitComponentChanged(); } }, @@ -630,28 +611,20 @@ function copyData (dest, sourceData) { * @param {boolean} isObjectBased - Whether values are objects. * @returns Overridden object or value. */ -<<<<<<< HEAD -function extendProperties (dest, source, isSinglePropSchema) { - if (isSinglePropSchema && (source === null || typeof source !== 'object')) { return source; } - var copy = utils.extend(dest, source); - var key; - for (key in copy) { - if (!copy[key] || copy[key].constructor !== Object) { continue; } - copy[key] = utils.clone(copy[key]); - } - return copy; -======= function extendProperties (dest, source, isObjectBased) { var key; - if (isObjectBased && source instanceof Object) { + if (isObjectBased && source.constructor === Object) { for (key in source) { if (source[key] === undefined) { continue; } - dest[key] = source[key]; + if (source[key] && source[key].constructor === Object) { + dest[key] = utils.clone(source[key]); + } else { + dest[key] = source[key]; + } } return dest; } return source; ->>>>>>> object pooling in component core, optimize style parser to reuse object } /** diff --git a/src/utils/styleParser.js b/src/utils/styleParser.js index 10ada4c3dbc..cfb1fd54854 100644 --- a/src/utils/styleParser.js +++ b/src/utils/styleParser.js @@ -3,6 +3,7 @@ * Some code adapted from `style-attr` (https://github.com/joshwnj/style-attr) * by Josh Johnston (MIT License). */ +var DASH_REGEX = /-([a-z])/g; /** * Deserialize style-like string into an object of properties. @@ -38,7 +39,7 @@ module.exports.stringify = function (data) { * @return {string} CamelCased string. */ function toCamelCase (str) { - return str.replace(/-([a-z])/g, upperCase); + return str.replace(DASH_REGEX, upperCase); } module.exports.toCamelCase = toCamelCase; diff --git a/tests/components/obj-model.test.js b/tests/components/obj-model.test.js index b58dee7f18b..cda162ae377 100644 --- a/tests/components/obj-model.test.js +++ b/tests/components/obj-model.test.js @@ -51,6 +51,32 @@ suite('obj-model', function () { el.setAttribute('obj-model', {mtl: `url(${MTL})`, obj: `url(${OBJ})`}); }); + test('can load .OBJ with material', function (done) { + var el = this.el; + el.setAttribute('material', 'color', 'red'); + el.addEventListener('object3dset', () => { + var material = el.getObject3D('mesh').children[0].material; + assert.equal(material.color.r, 1); + done(); + }); + el.setAttribute('obj-model', 'obj', '#obj'); + }); +}); + +suite('multiple OBJ', function () { + setup(function (done) { + var el; + var objAsset = document.createElement('a-asset-item'); + var mtlAsset = document.createElement('a-asset-item'); + mtlAsset.setAttribute('id', 'mtl'); + mtlAsset.setAttribute('src', MTL); + objAsset.setAttribute('id', 'obj'); + objAsset.setAttribute('src', OBJ); + el = this.el = entityFactory({assets: [mtlAsset, objAsset]}); + if (el.hasLoaded) { done(); } + el.addEventListener('loaded', function () { done(); }); + }); + test('can load multiple .OBJ', function (done) { var el = this.el; var el2 = document.createElement('a-entity'); @@ -74,19 +100,4 @@ suite('obj-model', function () { }); el.sceneEl.appendChild(el2); }); - - test('can load .OBJ with material', function (done) { - var parentEl = this.el; - parentEl.addEventListener('child-attached', function (evt) { - var el = evt.detail.el; - el.addEventListener('model-loaded', function () { - var material = el.getObject3D('mesh').children[0].material; - assert.equal(material.color.r, 1); - assert.equal(material.metalness, 0.123); - done(); - }); - }); - parentEl.innerHTML = ''; - }); }); diff --git a/tests/core/a-entity.test.js b/tests/core/a-entity.test.js index ff931360115..9fd26907106 100644 --- a/tests/core/a-entity.test.js +++ b/tests/core/a-entity.test.js @@ -466,7 +466,6 @@ suite('a-entity', function () { suite('flushToDOM', function () { test('updates DOM attributes', function () { - var el = this.el; var material; var materialStr = 'color: #F0F; metalness: 0.75'; el.setAttribute('material', materialStr); @@ -478,7 +477,6 @@ suite('a-entity', function () { }); test('updates DOM attributes of a multiple component', function () { - var el = this.el; var soundAttrValue; var soundStr = 'src: url(mysoundfile.mp3); autoplay: true'; el.setAttribute('sound__1', {'src': 'url(mysoundfile.mp3)', autoplay: true}); @@ -907,16 +905,7 @@ suite('a-entity', function () { assert.equal(el.getAttribute('material').color, '#FFF'); }); - test('does not remove default component', function () { - var el = this.el; - assert.ok('position' in el.components); - el.removeAttribute('position'); - assert.shallowDeepEqual(el.getDOMAttribute('position'), {}); - assert.ok('position' in el.components); - }); - test('does not remove mixed-in component', function () { - var el = this.el; mixinFactory('foo', {position: '1 2 3'}); mixinFactory('bar', {scale: '1 2 3'}); el.setAttribute('mixin', 'foo bar'); @@ -934,8 +923,6 @@ suite('a-entity', function () { suite('initComponent', function () { test('initializes component', function () { - var el = this.el; - el.initComponent('material', false, 'color: #F0F; transparent: true'); el.initComponent('material', 'color: #F0F; transparent: true', false); assert.ok(el.components.material); }); @@ -949,7 +936,6 @@ suite('a-entity', function () { }); test('initializes dependency component and can set attribute', function () { - var el = this.el; el.initComponent('material', '', true); assert.shallowDeepEqual(el.getAttribute('material'), {}); }); diff --git a/tests/core/component.test.js b/tests/core/component.test.js index f98fdabf374..672fdada6ff 100644 --- a/tests/core/component.test.js +++ b/tests/core/component.test.js @@ -289,25 +289,28 @@ suite('Component', function () { }); test('does not emit componentchanged for multi-prop if not changed', function (done) { - el.addEventListener('componentinitialized', function (evt) { + function componentChanged (evt) { if (evt.detail.name !== 'material') { return; } + el.removeEventListener('componentchanged', componentChanged); + // Should not reach here. + assert.equal(true, false, 'Component should not have emitted changed.'); + } - el.addEventListener('componentchanged', function (evt) { - if (evt.detail.name !== 'material') { return; } - // Should not reach here. - assert.equal(true, false, 'Component should not have emitted changed.'); - }); - - // Update. + function componentInitialized (evt) { + if (evt.detail.name !== 'material') { return; } + el.addEventListener('componentchanged', componentChanged); el.setAttribute('material', 'color', 'red'); + } - // Have `done()` race with the failing assertion in the event handler. - setTimeout(() => { - done(); - }, 100); - }); - // Initialization. + el.addEventListener('componentinitialized', componentInitialized); el.setAttribute('material', 'color', 'red'); + + // Have `done()` race with the failing assertion in the event handler. + setTimeout(() => { + el.removeEventListener('componentchanged', componentChanged); + el.removeEventListener('componentinitialized', componentInitialized); + done(); + }, 100); }); test('does not update for multi-prop if not changed', function (done) { @@ -358,25 +361,27 @@ suite('Component', function () { }); test('does not emit componentchanged for single-prop if not changed', function (done) { - el.addEventListener('componentinitialized', function (evt) { - if (evt.detail.name !== 'position') { return; } + function componentInitialized (evt) { + if (evt.detail.name !== 'visible') { return; } + el.addEventListener('componentchanged', componentChanged); + el.setAttribute('visible', false); + } - el.addEventListener('componentchanged', function (evt) { - if (evt.detail.name !== 'position') { return; } - // Should not reach here. - assert.equal(true, false, 'Component should not have emitted changed.'); - }); + function componentChanged (evt) { + if (evt.detail.name !== 'visible') { return; } + // Should not reach here. + assert.equal(true, false, 'Component should not have emitted changed.'); + } - // Update. - el.setAttribute('position', {x: 1, y: 2, z: 3}); + el.addEventListener('componentinitialized', componentInitialized); + el.setAttribute('visible', false); - // Have `done()` race with the failing assertion in the event handler. - setTimeout(() => { - done(); - }, 100); - }); - // Initialization. - el.setAttribute('position', {x: 1, y: 2, z: 3}); + // Have `done()` race with the failing assertion in the event handler. + setTimeout(() => { + el.removeEventListener('componentinitialized', componentInitialized); + el.removeEventListener('componentchanged', componentChanged); + done(); + }, 100); }); test('does not emit componentchanged for value if not changed', function (done) { @@ -437,8 +442,6 @@ suite('Component', function () { }); test('does not clone properties from attrValue into data that are not plain objects', function () { - var attrValue; - var data; var el; registerComponent('dummy', { schema: { @@ -458,6 +461,19 @@ suite('Component', function () { var el; var data; + registerComponent('dummy', { + schema: { + color: {type: 'string'}, + direction: {type: 'vec3'}, + el: {type: 'selector', default: 'body'} + } + }); + + el = document.createElement('a-entity'); + el.hasLoaded = true; + el.setAttribute('dummy', ''); + assert.notOk(el.components.dummy.attrValue.el); + // Direction property preserved across updateProperties calls but cloned into a different // object. el.components.dummy.updateProperties({ @@ -702,8 +718,8 @@ suite('Component', function () { suite('updateProperties', function () { setup(function (done) { - components.dummy = undefined; var el = this.el = entityFactory(); + components.dummy = undefined; if (el.hasLoaded) { done(); } el.addEventListener('loaded', function () { done(); }); }); @@ -749,7 +765,7 @@ suite('Component', function () { el.addEventListener('loaded', function () { done(); }); }); - test('init is only called once if the init routine sets the component', function () { + test('init is called once if the init routine sets the component', function () { var initCanaryStub = sinon.stub(); var el = this.el; registerComponent('dummy', { @@ -1016,15 +1032,19 @@ suite('Component', function () { test('applies default array property types with no defined value', function (done) { var el; registerComponent('test', { - schema: {default: ['foo']}, + schema: { + arr: {default: ['foo']} + }, update: function () { - assert.equal(this.data[0], 'foo'); + assert.equal(this.data.arr[0], 'foo'); done(); } }); el = entityFactory(); - el.setAttribute('test', ''); + el.addEventListener('loaded', () => { + el.setAttribute('test', ''); + }); }); }); diff --git a/tests/core/controls.test.js b/tests/core/controls.test.js index 77d500db570..59cac49a8c6 100644 --- a/tests/core/controls.test.js +++ b/tests/core/controls.test.js @@ -1,4 +1,4 @@ -/* global AFRAME, Event, assert, process, setup, suite, test */ +/* global Event, assert, process, setup, suite, test */ var helpers = require('../helpers'); var PI = Math.PI; diff --git a/tests/core/shader.test.js b/tests/core/shader.test.js index 83ad656eb0b..77429b03f57 100644 --- a/tests/core/shader.test.js +++ b/tests/core/shader.test.js @@ -1,32 +1,35 @@ /* global AFRAME, assert, process, setup, suite, test, teardown */ -var entityFactory = require('../helpers').entityFactory; +const entityFactory = require('../helpers').entityFactory; -var THREE = require('lib/three'); -var VIDEO = 'base/tests/assets/test.mp4'; +const THREE = require('lib/three'); +const VIDEO = 'base/tests/assets/test.mp4'; suite('shader', function () { + var el; + setup(function (done) { - this.el = entityFactory(); - if (this.el.hasLoaded) { + el = entityFactory(); + if (el.hasLoaded) { done(); } else { - this.el.addEventListener('loaded', function () { done(); }); + el.addEventListener('loaded', function () { done(); }); } }); teardown(function () { - var el = this.el; + el.removeAttribute('material'); if (el.components.material) { el.components.material.remove(); } if (AFRAME.shaders.testShader) { delete AFRAME.shaders.testShader; } }); suite('registerShader', function () { + var shader; + setup(function () { - this.shader = AFRAME.registerShader('testShader', {}); + shader = AFRAME.registerShader('testShader', {}); }); test('shader prototype default methods and properties', function () { - var shader = this.shader; assert.ok(shader); assert.ok(shader.prototype.init); assert.ok(shader.prototype.update); @@ -37,11 +40,9 @@ suite('shader', function () { }); test('shader instance receives methods and properties', function () { - var shader = this.shader; - var el = this.el; el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; + const material = el.components.material; + const instance = material.shader; assert.ok(material); assert.ok(instance); assert.ok(instance.material instanceof THREE.ShaderMaterial); @@ -56,17 +57,15 @@ suite('shader', function () { }); test('shader instance called init and update', function () { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); + const initSpy = this.sinon.spy(shader.prototype, 'init'); + const updateSpy = this.sinon.spy(shader.prototype, 'update'); el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; - assert.ok(material); - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); + const material = el.components.material; + const instance = material.shader; + assert.ok(material, 'material'); + assert.ok(instance, 'shader'); + assert.ok(initSpy.calledOnce, 'init called once'); + assert.ok(updateSpy.calledOnce, 'update called once'); }); }); @@ -76,163 +75,157 @@ suite('shader', function () { }); test('shader instance receives methods and properties', function () { - var el = this.el; el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; + const material = el.components.material; + const instance = material.shader; assert.ok(material); assert.ok(instance); assert.ok(instance.material instanceof THREE.RawShaderMaterial); }); }); +}); - suite('data binding', function () { - setup(function () { - this.shader = AFRAME.registerShader('testShader', { - schema: { - src: {type: 'map', is: 'uniform'}, - otherMap: {type: 'map', is: 'uniform'}, - vec2Uniform: {type: 'vec2', default: {x: 1, y: 2}, is: 'uniform'}, - vec2Attribute: {type: 'vec2', default: {x: 3, y: 4}, is: 'attribute'}, - vec2Neither: {type: 'vec2', default: {x: 5, y: 6}} - } - }); - }); +suite('shader data binding', function () { + var el; + var initSpy; + var shader; + var updateSpy; - teardown(function () { - var el = this.el; - if (el.components.material) { el.components.material.remove(); } - if (AFRAME.shaders.testShader) { delete AFRAME.shaders.testShader; } + setup(function (done) { + AFRAME.registerShader('testShader', { + schema: { + src: {type: 'map', is: 'uniform'}, + otherMap: {type: 'map', is: 'uniform'}, + vec2Uniform: {type: 'vec2', default: {x: 1, y: 2}, is: 'uniform'}, + vec2Attribute: {type: 'vec2', default: {x: 3, y: 4}, is: 'attribute'}, + vec2Neither: {type: 'vec2', default: {x: 5, y: 6}} + } }); - test('src parameter of type map --> uniform src, not attribute', function () { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - // The value won't be assigned until the texture loads. - assert.ok(instance.uniforms['src']); - assert.notOk(instance.attributes && (instance.attributes['map'] || - instance.attributes['src'])); - }); + shader = AFRAME.shaders.testShader.Shader; + initSpy = this.sinon.spy(shader.prototype, 'init'); + updateSpy = this.sinon.spy(shader.prototype, 'update'); - test('src loads inline video', function (done) { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - // With Travis CI, the actual videos are never loaded, - // so check for materialtextureloaded not materialvideoloadeddata, - // and don't try to assert the uniform values - el.addEventListener('materialtextureloaded', function () { - var material = el.components.material; - var instance = material.shader; - assert.equal(instance.material['_texture_src'].image.getAttribute('src'), VIDEO); - done(); - }); - el.setAttribute('material', 'shader: testShader; src:' + VIDEO); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - // The value won't be assigned until the texture loads. - assert.ok(instance.uniforms['src']); - assert.notOk(instance.attributes && (instance.attributes['map'] || - instance.attributes['src'])); - }); + el = entityFactory(); + if (el.hasLoaded) { + done(); + } else { + el.addEventListener('loaded', function () { done(); }); + } + }); - test('otherMap loads inline video', function (done) { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - // With Travis CI, the actual videos are never loaded, - // so check for materialtextureloaded not materialvideoloadeddata, - // and don't try to assert the uniform values. - el.addEventListener('materialtextureloaded', function () { - var material = el.components.material; - var instance = material.shader; - assert.equal(instance.material['_texture_' + 'otherMap'].image.getAttribute('src'), - VIDEO); - done(); - }); - el.setAttribute('material', 'shader: testShader; otherMap:' + VIDEO); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - assert.ok(instance.uniforms['otherMap']); - // The value won't be assigned until the texture loads. - assert.notOk(instance.attributes && instance.attributes['otherMap']); - }); + teardown(function () { + el.removeAttribute('material'); + if (AFRAME.shaders.testShader) { delete AFRAME.shaders.testShader; } + }); - test('vec2Uniform parameter --> uniform vec2Uniform, not attribute', function () { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - assert.ok(instance.uniforms['vec2Uniform']); - assert.equal(instance.uniforms['vec2Uniform'].value.x, 1); - assert.equal(instance.uniforms['vec2Uniform'].value.y, 2); - assert.notOk(instance.attributes['vec2Uniform']); - }); + test('src parameter of type map is uniform', function () { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + el.setAttribute('material', 'shader: testShader'); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + // The value won't be assigned until the texture loads. + assert.ok(instance.uniforms['src']); + assert.notOk(instance.attributes && (instance.attributes['map'] || + instance.attributes['src'])); + }); - test('vec2Attribute parameter --> attribute vec2Attribute, not uniform', function () { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - assert.ok(instance.attributes['vec2Attribute']); - assert.equal(instance.attributes['vec2Attribute'].value.x, 3); - assert.equal(instance.attributes['vec2Attribute'].value.y, 4); - assert.notOk(instance.uniforms['vec2Attribute']); - }); + test('src loads inline video', function (done) { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + // With Travis CI, the actual videos are never loaded, + // so check for materialtextureloaded not materialvideoloadeddata, + // and don't try to assert the uniform values + el.addEventListener('materialtextureloaded', materialTextureLoaded); + function materialTextureLoaded () { + const material = el.components.material; + const instance = material.shader; + assert.equal(instance.material['_texture_src'].image.getAttribute('src'), VIDEO); + el.removeEventListener('materialtextureloaded', materialTextureLoaded); + done(); + } + el.setAttribute('material', 'shader: testShader; src:' + VIDEO); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + // The value won't be assigned until the texture loads. + assert.ok(instance.uniforms['src']); + assert.notOk(instance.attributes && (instance.attributes['map'] || + instance.attributes['src'])); + }); - test('vec2Neither parameter --> neither uniform nor attribute', function () { - var shader = this.shader; - var el = this.el; - var initSpy = this.sinon.spy(shader.prototype, 'init'); - var updateSpy = this.sinon.spy(shader.prototype, 'update'); - assert.notOk(initSpy.called); - assert.notOk(updateSpy.called); - el.setAttribute('material', 'shader: testShader'); - var material = el.components.material; - var instance = material.shader; - assert.ok(instance); - assert.ok(initSpy.calledOnce); - assert.ok(updateSpy.calledOnce); - assert.notOk(instance.attributes['vec2Neither']); - assert.notOk(instance.uniforms['vec2Neither']); - }); + test('otherMap loads inline video', function (done) { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + // With Travis CI, the actual videos are never loaded, + // so check for materialtextureloaded not materialvideoloadeddata, + // and don't try to assert the uniform values. + el.addEventListener('materialtextureloaded', materialTextureLoaded); + function materialTextureLoaded () { + const material = el.components.material; + const instance = material.shader; + assert.equal(instance.material['_texture_' + 'otherMap'].image.getAttribute('src'), + VIDEO); + el.removeEventListener('materialtextureloaded', materialTextureLoaded); + done(); + } + el.setAttribute('material', 'shader: testShader; otherMap:' + VIDEO); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + assert.ok(instance.uniforms['otherMap']); + // The value won't be assigned until the texture loads. + assert.notOk(instance.attributes && instance.attributes['otherMap']); + }); + + test('vec2Uniform parameter is uniform', function () { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + el.setAttribute('material', 'shader: testShader'); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + assert.ok(instance.uniforms['vec2Uniform']); + assert.equal(instance.uniforms['vec2Uniform'].value.x, 1); + assert.equal(instance.uniforms['vec2Uniform'].value.y, 2); + assert.notOk(instance.attributes['vec2Uniform']); + }); + + test('vec2Attribute parameter is attribute', function () { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + el.setAttribute('material', 'shader: testShader'); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + assert.ok(instance.attributes['vec2Attribute']); + assert.equal(instance.attributes['vec2Attribute'].value.x, 3); + assert.equal(instance.attributes['vec2Attribute'].value.y, 4); + assert.notOk(instance.uniforms['vec2Attribute']); + }); + + test('vec2Neither parameter is neither uniform nor attribute', function () { + assert.notOk(initSpy.called); + assert.notOk(updateSpy.called); + el.setAttribute('material', 'shader: testShader'); + const material = el.components.material; + const instance = material.shader; + assert.ok(instance); + assert.ok(initSpy.calledOnce); + assert.ok(updateSpy.calledOnce); + assert.notOk(instance.attributes['vec2Neither']); + assert.notOk(instance.uniforms['vec2Neither']); }); }); From f87c29672abc40324f81d1f2915d7ab78b2cef8a Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Wed, 26 Sep 2018 03:59:36 -0700 Subject: [PATCH 5/6] fix obj-model calling .remove from another handler to fix tests --- src/components/obj-model.js | 6 ++++- tests/components/obj-model.test.js | 38 ++++++++++-------------------- 2 files changed, 17 insertions(+), 27 deletions(-) diff --git a/src/components/obj-model.js b/src/components/obj-model.js index 7c842d8967d..890dd8aae72 100644 --- a/src/components/obj-model.js +++ b/src/components/obj-model.js @@ -21,12 +21,16 @@ module.exports.Component = registerComponent('obj-model', { update: function () { var data = this.data; if (!data.obj) { return; } - this.remove(); + this.resetMesh(); this.loadObj(data.obj, data.mtl); }, remove: function () { if (!this.model) { return; } + this.resetMesh(); + }, + + resetMesh: function () { this.el.removeObject3D('mesh'); }, diff --git a/tests/components/obj-model.test.js b/tests/components/obj-model.test.js index cda162ae377..2410644b733 100644 --- a/tests/components/obj-model.test.js +++ b/tests/components/obj-model.test.js @@ -51,35 +51,10 @@ suite('obj-model', function () { el.setAttribute('obj-model', {mtl: `url(${MTL})`, obj: `url(${OBJ})`}); }); - test('can load .OBJ with material', function (done) { - var el = this.el; - el.setAttribute('material', 'color', 'red'); - el.addEventListener('object3dset', () => { - var material = el.getObject3D('mesh').children[0].material; - assert.equal(material.color.r, 1); - done(); - }); - el.setAttribute('obj-model', 'obj', '#obj'); - }); -}); - -suite('multiple OBJ', function () { - setup(function (done) { - var el; - var objAsset = document.createElement('a-asset-item'); - var mtlAsset = document.createElement('a-asset-item'); - mtlAsset.setAttribute('id', 'mtl'); - mtlAsset.setAttribute('src', MTL); - objAsset.setAttribute('id', 'obj'); - objAsset.setAttribute('src', OBJ); - el = this.el = entityFactory({assets: [mtlAsset, objAsset]}); - if (el.hasLoaded) { done(); } - el.addEventListener('loaded', function () { done(); }); - }); - test('can load multiple .OBJ', function (done) { var el = this.el; var el2 = document.createElement('a-entity'); + var elPromise = new Promise(function (resolve) { el.addEventListener('model-loaded', resolve); }); @@ -100,4 +75,15 @@ suite('multiple OBJ', function () { }); el.sceneEl.appendChild(el2); }); + + test('can load .OBJ with material', function (done) { + var el = this.el; + el.setAttribute('material', 'color', 'red'); + el.addEventListener('object3dset', () => { + var material = el.getObject3D('mesh').children[0].material; + assert.equal(material.color.r, 1); + done(); + }); + el.setAttribute('obj-model', 'obj', '#obj'); + }); }); From 784aafb0193a7b14417481166a1a608aae3e15fd Mon Sep 17 00:00:00 2001 From: Kevin Ngo Date: Sun, 30 Sep 2018 10:54:54 -0700 Subject: [PATCH 6/6] remove unused savedPose from camera.js --- src/components/camera.js | 5 ----- tests/components/geometry.test.js | 1 - 2 files changed, 6 deletions(-) diff --git a/src/components/camera.js b/src/components/camera.js index e097848c9c9..a32599ecb46 100644 --- a/src/components/camera.js +++ b/src/components/camera.js @@ -79,11 +79,6 @@ module.exports.Component = registerComponent('camera', { // Camera disabled. Set camera to another camera. system.disableSpectatorCamera(); } - - this.savedPose = { - position: el.getAttribute('position'), - rotation: el.getAttribute('rotation') - }; }, /** diff --git a/tests/components/geometry.test.js b/tests/components/geometry.test.js index 5913490c2fb..b5ee6e70c38 100644 --- a/tests/components/geometry.test.js +++ b/tests/components/geometry.test.js @@ -18,7 +18,6 @@ suite('geometry', function () { suite('update', function () { test('allows empty geometry', function () { this.el.setAttribute('geometry', ''); - this.el.setAttribute('geometry', ''); }); test('creates geometry', function () {