From 3d40884371dbf8c23cf5c5d86155945281097124 Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Fri, 30 Aug 2024 20:30:00 -0400 Subject: [PATCH 1/6] refactor: replace push.apply with for loop --- .../engine/Source/Renderer/ShaderBuilder.js | 24 ++++++++++++++----- .../Source/Scene/Cesium3DTileBatchTable.js | 11 +++++---- .../engine/Source/Scene/Cesium3DTileStyle.js | 12 +++++++--- .../Source/Scene/ConditionsExpression.js | 8 +++++-- packages/engine/Source/Scene/GltfLoader.js | 18 ++++++++++++-- .../Source/Scene/MetadataTableProperty.js | 19 +++++++-------- 6 files changed, 64 insertions(+), 28 deletions(-) diff --git a/packages/engine/Source/Renderer/ShaderBuilder.js b/packages/engine/Source/Renderer/ShaderBuilder.js index 6fd79ab0098e..93d8df2b7850 100644 --- a/packages/engine/Source/Renderer/ShaderBuilder.js +++ b/packages/engine/Source/Renderer/ShaderBuilder.js @@ -414,7 +414,9 @@ ShaderBuilder.prototype.addVertexLines = function (lines) { const vertexLines = this._vertexShaderParts.shaderLines; if (Array.isArray(lines)) { - vertexLines.push.apply(vertexLines, lines); + for (let i = 0; i < lines.length; i++) { + vertexLines.push(lines[i]); + } } else { // Single string case vertexLines.push(lines); @@ -449,7 +451,9 @@ ShaderBuilder.prototype.addFragmentLines = function (lines) { const fragmentLines = this._fragmentShaderParts.shaderLines; if (Array.isArray(lines)) { - fragmentLines.push.apply(fragmentLines, lines); + for (let i = 0; i < lines.length; i++) { + fragmentLines.push(lines[i]); + } } else { // Single string case fragmentLines.push(lines); @@ -534,7 +538,9 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - vertexLines.push.apply(vertexLines, structLines); + for (let j = 0; j < structLines.length; j++) { + vertexLines.push(structLines[j]); + } } structIds = shaderBuilder._fragmentShaderParts.structIds; @@ -542,7 +548,9 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - fragmentLines.push.apply(fragmentLines, structLines); + for (let j = 0; j < structLines.length; j++) { + fragmentLines.push(structLines[j]); + } } return { @@ -577,7 +585,9 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - vertexLines.push.apply(vertexLines, functionLines); + for (let j = 0; j < functionLines.length; j++) { + vertexLines.push(functionLines[j]); + } } functionIds = shaderBuilder._fragmentShaderParts.functionIds; @@ -585,7 +595,9 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - fragmentLines.push.apply(fragmentLines, functionLines); + for (let j = 0; j < functionLines.length; j++) { + fragmentLines.push(functionLines[j]); + } } return { diff --git a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js index b9a678cdd8c5..f0ea897137b6 100644 --- a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js +++ b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js @@ -373,13 +373,14 @@ Cesium3DTileBatchTable.prototype.getPropertyIds = function (batchId, results) { results.length = 0; const scratchPropertyIds = Object.keys(this._properties); - results.push.apply(results, scratchPropertyIds); + for (let i = 0; i < scratchPropertyIds.length; ++i) { + results.push(scratchPropertyIds[i]); + } if (defined(this._batchTableHierarchy)) { - results.push.apply( - results, - this._batchTableHierarchy.getPropertyIds(batchId, scratchPropertyIds) - ); + for (let i = 0; i < this._batchTableHierarchy._classes.length; ++i) { + results.push(this._batchTableHierarchy._classes[i].name); + } } return results; diff --git a/packages/engine/Source/Scene/Cesium3DTileStyle.js b/packages/engine/Source/Scene/Cesium3DTileStyle.js index b5cd56c60d37..92cbb8097b19 100644 --- a/packages/engine/Source/Scene/Cesium3DTileStyle.js +++ b/packages/engine/Source/Scene/Cesium3DTileStyle.js @@ -1453,15 +1453,21 @@ Cesium3DTileStyle.prototype.getVariables = function () { let variables = []; if (defined(this.color) && defined(this.color.getVariables)) { - variables.push.apply(variables, this.color.getVariables()); + for (let i = 0; i < this.color.getVariables().length; i++) { + variables.push(this.color.getVariables()[i]); + } } if (defined(this.show) && defined(this.show.getVariables)) { - variables.push.apply(variables, this.show.getVariables()); + for (let i = 0; i < this.show.getVariables().length; i++) { + variables.push(this.show.getVariables()[i]); + } } if (defined(this.pointSize) && defined(this.pointSize.getVariables)) { - variables.push.apply(variables, this.pointSize.getVariables()); + for (let i = 0; i < this.pointSize.getVariables().length; i++) { + variables.push(this.pointSize.getVariables()[i]); + } } // Remove duplicates diff --git a/packages/engine/Source/Scene/ConditionsExpression.js b/packages/engine/Source/Scene/ConditionsExpression.js index 1bed27442ace..21ab50ba3d58 100644 --- a/packages/engine/Source/Scene/ConditionsExpression.js +++ b/packages/engine/Source/Scene/ConditionsExpression.js @@ -203,8 +203,12 @@ ConditionsExpression.prototype.getVariables = function () { const length = conditions.length; for (let i = 0; i < length; ++i) { const statement = conditions[i]; - variables.push.apply(variables, statement.condition.getVariables()); - variables.push.apply(variables, statement.expression.getVariables()); + for (const variable of statement.condition.getVariables()) { + variables.push(variable); + } + for (const variable of statement.expression.getVariables()) { + variables.push(variable); + } } // Remove duplicates diff --git a/packages/engine/Source/Scene/GltfLoader.js b/packages/engine/Source/Scene/GltfLoader.js index 18f825a6ae4e..0588f395f5a7 100644 --- a/packages/engine/Source/Scene/GltfLoader.js +++ b/packages/engine/Source/Scene/GltfLoader.js @@ -2683,12 +2683,26 @@ function parse(loader, frameState) { // Gather promises and handle any errors const readyPromises = []; - readyPromises.push.apply(readyPromises, loader._loaderPromises); + for (let i = 0; i < loader._loaderPromises.length; ++i) { + const promise = loader._loaderPromises[i]; + promise.then(undefined, function (error) { + loader._state = GltfLoaderState.FAILED; + loader._error = error; + }); + readyPromises.push(promise); + } // When incrementallyLoadTextures is true, the errors are caught and thrown individually // since it doesn't affect the overall loader state if (!loader._incrementallyLoadTextures) { - readyPromises.push.apply(readyPromises, loader._texturesPromises); + for (let i = 0; i < loader._textureLoaders.length; ++i) { + const promise = loader._textureLoaders[i].readyPromise; + promise.then(undefined, function (error) { + loader._state = GltfLoaderState.FAILED; + loader._error = error; + }); + readyPromises.push(promise); + } } return Promise.all(readyPromises); diff --git a/packages/engine/Source/Scene/MetadataTableProperty.js b/packages/engine/Source/Scene/MetadataTableProperty.js index 8198e0151652..953b2853341e 100644 --- a/packages/engine/Source/Scene/MetadataTableProperty.js +++ b/packages/engine/Source/Scene/MetadataTableProperty.js @@ -388,7 +388,9 @@ function flatten(values) { for (let i = 0; i < values.length; i++) { const value = values[i]; if (Array.isArray(value)) { - result.push.apply(result, value); + for (let j = 0; j < value.length; j++) { + result.push(value[j]); + } } else { result.push(value); } @@ -562,7 +564,7 @@ function getInt64NumberFallback(index, values) { function getInt64BigIntFallback(index, values) { const dataView = values.dataView; const byteOffset = index * 8; - // eslint-disable-next-line no-undef + let value = BigInt(0); const isNegative = (dataView.getUint8(byteOffset + 7) & 0x80) > 0; let carrying = true; @@ -578,7 +580,7 @@ function getInt64BigIntFallback(index, values) { byte = ~byte & 0xff; } } - value += BigInt(byte) * (BigInt(1) << BigInt(i * 8)); // eslint-disable-line + value += BigInt(byte) * (BigInt(1) << BigInt(i * 8)); } if (isNegative) { value = -value; @@ -605,14 +607,13 @@ function getUint64BigIntFallback(index, values) { const byteOffset = index * 8; // Split 64-bit number into two 32-bit (4-byte) parts - // eslint-disable-next-line no-undef + const left = BigInt(dataView.getUint32(byteOffset, true)); - // eslint-disable-next-line no-undef const right = BigInt(dataView.getUint32(byteOffset + 4, true)); // Combine the two 32-bit values - // eslint-disable-next-line no-undef + const value = left + BigInt(4294967296) * right; return value; @@ -783,7 +784,6 @@ function BufferView(bufferView, componentType, length) { return getInt64BigIntFallback(index, that); }; } else { - // eslint-disable-next-line typedArray = new BigInt64Array( bufferView.buffer, bufferView.byteOffset, @@ -791,7 +791,7 @@ function BufferView(bufferView, componentType, length) { ); setFunction = function (index, value) { // Convert the number to a BigInt before setting the value in the typed array - that.typedArray[index] = BigInt(value); // eslint-disable-line + that.typedArray[index] = BigInt(value); }; } } else if (componentType === MetadataComponentType.UINT64) { @@ -817,7 +817,6 @@ function BufferView(bufferView, componentType, length) { return getUint64BigIntFallback(index, that); }; } else { - // eslint-disable-next-line typedArray = new BigUint64Array( bufferView.buffer, bufferView.byteOffset, @@ -825,7 +824,7 @@ function BufferView(bufferView, componentType, length) { ); setFunction = function (index, value) { // Convert the number to a BigInt before setting the value in the typed array - that.typedArray[index] = BigInt(value); // eslint-disable-line + that.typedArray[index] = BigInt(value); }; } } else { From 859c5c27e973de0d3a8cbe092d7c0b4994c94f87 Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Fri, 30 Aug 2024 20:55:38 -0400 Subject: [PATCH 2/6] refactor: change push.apply to for loops --- .../codemirror-5.52.0/lib/codemirror.js | 4 +++- .../codemirror-5.52.0/src/model/chunk.js | 4 +++- .../dojo-release-1.10.4/dojo/Evented.js | 4 +++- .../Model/ClassificationModelDrawCommand.js | 24 ++++++++++++++----- .../Source/Scene/Model/GeoJsonLoader.js | 6 ++++- .../Scene/Model/InstancingPipelineStage.js | 14 +++++------ .../Source/Scene/Model/ModelSceneGraph.js | 5 +++- packages/engine/Source/Scene/PropertyTable.js | 23 +++++++----------- .../ClassificationModelDrawCommandSpec.js | 9 +++++-- .../Model/ClassificationPipelineStageSpec.js | 4 +++- .../Source/Animation/AnimationViewModel.js | 5 +++- 11 files changed, 65 insertions(+), 37 deletions(-) diff --git a/ThirdParty/codemirror-5.52.0/lib/codemirror.js b/ThirdParty/codemirror-5.52.0/lib/codemirror.js index 9489e39c3a48..308a50dc97f0 100644 --- a/ThirdParty/codemirror-5.52.0/lib/codemirror.js +++ b/ThirdParty/codemirror-5.52.0/lib/codemirror.js @@ -5604,7 +5604,9 @@ // Helper used to collapse a small branch into a single leaf. collapse: function(lines) { - lines.push.apply(lines, this.lines); + for (let i = 0; i < this.lines.length; i++) { + lines.push(this.lines[i]); + } }, // Insert the given array of lines at offset 'at', count them as diff --git a/ThirdParty/codemirror-5.52.0/src/model/chunk.js b/ThirdParty/codemirror-5.52.0/src/model/chunk.js index d82716ded4c1..c4dbc8187a58 100644 --- a/ThirdParty/codemirror-5.52.0/src/model/chunk.js +++ b/ThirdParty/codemirror-5.52.0/src/model/chunk.js @@ -42,7 +42,9 @@ LeafChunk.prototype = { // Helper used to collapse a small branch into a single leaf. collapse(lines) { - lines.push.apply(lines, this.lines) + for (let i = 0; i < this.lines.length; ++i){ + lines.push(this.lines[i]) + } }, // Insert the given array of lines at offset 'at', count them as diff --git a/ThirdParty/dojo-release-1.10.4/dojo/Evented.js b/ThirdParty/dojo-release-1.10.4/dojo/Evented.js index 208a2a231d71..0083a4f78ca1 100644 --- a/ThirdParty/dojo-release-1.10.4/dojo/Evented.js +++ b/ThirdParty/dojo-release-1.10.4/dojo/Evented.js @@ -28,7 +28,9 @@ define(["./aspect", "./on"], function(aspect, on){ }, emit: function(type, event){ var args = [this]; - args.push.apply(args, arguments); + for (var i = 0; i < arguments.length; i++){ + args.push(arguments[i]); + } return on.emit.apply(on, args); } }; diff --git a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js index 4d23fa50ef79..836192cddbf4 100644 --- a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js +++ b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js @@ -482,16 +482,22 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( const passes = frameState.passes; if (passes.render) { if (this._useDebugWireframe) { - result.push.apply(result, this._commandListDebugWireframe); + for (let i = 0; i < this._commandListDebugWireframe.length; i++) { + result.push(this._commandListDebugWireframe[i]); + } return; } if (this._classifiesTerrain) { - result.push.apply(result, this._commandListTerrain); + for (let i = 0; i < this._commandListTerrain.length; i++) { + result.push(this._commandListTerrain[i]); + } } if (this._classifies3DTiles) { - result.push.apply(result, this._commandList3DTiles); + for (let i = 0; i < this._commandList3DTiles.length; i++) { + result.push(this._commandList3DTiles[i]); + } } const useIgnoreShowCommands = @@ -513,17 +519,23 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( ); } - result.push.apply(result, this._commandListIgnoreShow); + for (let i = 0; i < this._commandListIgnoreShow.length; i++) { + result.push(this._commandListIgnoreShow[i]); + } } } if (passes.pick) { if (this._classifiesTerrain) { - result.push.apply(result, this._commandListTerrainPicking); + for (let i = 0; i < this._commandListTerrainPicking.length; i++) { + result.push(this._commandListTerrainPicking[i]); + } } if (this._classifies3DTiles) { - result.push.apply(result, this._commandList3DTilesPicking); + for (let i = 0; i < this._commandList3DTilesPicking.length; i++) { + result.push(this._commandList3DTilesPicking[i]); + } } } diff --git a/packages/engine/Source/Scene/Model/GeoJsonLoader.js b/packages/engine/Source/Scene/Model/GeoJsonLoader.js index 5bc67d5ec9ca..7811132245b1 100644 --- a/packages/engine/Source/Scene/Model/GeoJsonLoader.js +++ b/packages/engine/Source/Scene/Model/GeoJsonLoader.js @@ -166,7 +166,11 @@ function parseMultiPolygon(coordinates) { const polygonsLength = coordinates.length; const lines = []; for (let i = 0; i < polygonsLength; i++) { - Array.prototype.push.apply(lines, parsePolygon(coordinates[i])); + const polygon = parsePolygon(coordinates[i]); + const polygonLength = polygon.length; + for (let j = 0; j < polygonLength; j++) { + lines.push(polygon[j]); + } } return lines; } diff --git a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js index f1c209285b93..5d4a0ff35dbb 100644 --- a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js +++ b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js @@ -210,10 +210,9 @@ InstancingPipelineStage.process = function (renderResources, node, frameState) { renderResources.uniformMap = combine(uniformMap, renderResources.uniformMap); renderResources.instanceCount = count; - renderResources.attributes.push.apply( - renderResources.attributes, - instancingVertexAttributes - ); + for (let i = 0; i < instancingVertexAttributes.length; i++) { + instancingVertexAttributes[i].instanceCount = count; + } }; const projectedTransformScratch = new Matrix4(); @@ -988,10 +987,9 @@ function processMatrixAttributes( shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row1`); shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row2`); - instancingVertexAttributes.push.apply( - instancingVertexAttributes, - matrixAttributes - ); + for (let i = 0; i < matrixAttributes.length; i++) { + instancingVertexAttributes.push(matrixAttributes[i]); + } } function processVec3Attribute( diff --git a/packages/engine/Source/Scene/Model/ModelSceneGraph.js b/packages/engine/Source/Scene/Model/ModelSceneGraph.js index 954c63081acc..11f69636f59f 100644 --- a/packages/engine/Source/Scene/Model/ModelSceneGraph.js +++ b/packages/engine/Source/Scene/Model/ModelSceneGraph.js @@ -911,7 +911,10 @@ ModelSceneGraph.prototype.pushDrawCommands = function (frameState) { pushDrawCommandOptions ); - frameState.commandList.push.apply(frameState.commandList, silhouetteCommands); + for (let i = 0; i < silhouetteCommands.length; i++) { + const silhouetteCommand = silhouetteCommands[i]; + silhouetteCommand.execute(frameState); + } }; // Callback is defined here to avoid allocating a closure in the render loop diff --git a/packages/engine/Source/Scene/PropertyTable.js b/packages/engine/Source/Scene/PropertyTable.js index 19860c82de18..841bfa45903c 100644 --- a/packages/engine/Source/Scene/PropertyTable.js +++ b/packages/engine/Source/Scene/PropertyTable.js @@ -280,8 +280,6 @@ PropertyTable.prototype.propertyExistsBySemantic = function (semantic) { return false; }; -const scratchResults = []; - /** * Returns an array of property IDs. For compatibility with the 3DTILES_batch_table_hierarchy extension, this is computed for a specific feature. * @@ -296,24 +294,21 @@ PropertyTable.prototype.getPropertyIds = function (index, results) { if (defined(this._metadataTable)) { // concat in place to avoid unnecessary array allocation - results.push.apply( - results, - this._metadataTable.getPropertyIds(scratchResults) - ); + for (let i = 0; i < this._metadataTable.propertyIds.length; i++) { + results.push(this._metadataTable.propertyIds[i]); + } } if (defined(this._batchTableHierarchy)) { - results.push.apply( - results, - this._batchTableHierarchy.getPropertyIds(index, scratchResults) - ); + for (let i = 0; i < this._batchTableHierarchy.propertyIds.length; i++) { + results.push(this._batchTableHierarchy.propertyIds[i]); + } } if (defined(this._jsonMetadataTable)) { - results.push.apply( - results, - this._jsonMetadataTable.getPropertyIds(scratchResults) - ); + for (let i = 0; i < this._jsonMetadataTable.propertyIds.length; i++) { + results.push(this._jsonMetadataTable.propertyIds[i]); + } } return results; diff --git a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js index 6df7bf6333e6..71cc2fc8031f 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js @@ -814,8 +814,13 @@ describe( expect(drawCommand.modelMatrix).toBe(command.modelMatrix); const commandList = []; - commandList.push.apply(commandList, drawCommand._commandListTerrain); - commandList.push.apply(commandList, drawCommand._commandList3DTiles); + for (let i = 0; i < drawCommand._commandListTerrain.length; i++) { + commandList.push(drawCommand._commandListTerrain[i]); + } + + for (let i = 0; i < drawCommand._commandList3DTiles.length; i++) { + commandList.push(drawCommand._commandList3DTiles[i]); + } const length = commandList.length; expect(length).toEqual(12); diff --git a/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js b/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js index dc30239df431..ca9f9236fc7e 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js @@ -19,7 +19,9 @@ describe("Scene/Model/ClassificationPipelineStage", function () { const batchLength = batchLengths[id]; const batch = new Array(batchLength); batch.fill(id); - featureIds.push.apply(featureIds, batch); + for (let j = 0; j < batch.length; j++) { + featureIds.push(batch[j]); + } } return featureIds; diff --git a/packages/widgets/Source/Animation/AnimationViewModel.js b/packages/widgets/Source/Animation/AnimationViewModel.js index 864b0bc51e6f..cf5e0b69c215 100644 --- a/packages/widgets/Source/Animation/AnimationViewModel.js +++ b/packages/widgets/Source/Animation/AnimationViewModel.js @@ -503,7 +503,10 @@ AnimationViewModel.prototype.setShuttleRingTicks = function (positiveTicks) { allTicks.push(-tick); } } - Array.prototype.push.apply(allTicks, sortedFilteredPositiveTicks); + for (i = 0, len = sortedFilteredPositiveTicks.length; i < len; ++i) { + tick = sortedFilteredPositiveTicks[i]; + allTicks.push(tick); + } this._allShuttleRingTicks = allTicks; }; From 108501927b2454693eac8070205be0bc9180673b Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Fri, 30 Aug 2024 21:05:51 -0400 Subject: [PATCH 3/6] chore: add name to contributors under individual --- CONTRIBUTORS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 5ffba76b8eba..1a99c3198644 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -405,3 +405,4 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu - [Levi Montgomery](https://github.com/Levi-Montgomery) - [Brandon Berisford](https://github.com/BeyondBelief96) - [Adam Wirth](https://https://github.com/adamwirth) +- [Tim Quattrochi](https://github.com/Tim-Quattrochi) From 8d30a9e84211b7772c0fd2953fafc443ec0b7045 Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Fri, 30 Aug 2024 21:16:43 -0400 Subject: [PATCH 4/6] chore: add to change log --- CHANGES.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 003d3a93908f..9373c816377a 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,13 @@ # Change Log +### 1.122 - 2024-08-30 + +#### @cesium/engine + +##### Fixes :wrench: + +- Replaced `Array.prototype.push.apply` with `for` loops to avoid exceeding the call stack limit. This change improves stability when handling large arrays. [#12053](https://github.com/CesiumGS/cesium/issues/12053) + ### 1.121 - 2024-09-01 #### @cesium/engine From 47994ebb10c3673b41fd21c78b71512467a440cd Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Mon, 23 Sep 2024 17:53:30 -0400 Subject: [PATCH 5/6] refactor: revert changes to ThirdParty code. --- ThirdParty/codemirror-5.52.0/lib/codemirror.js | 4 +--- ThirdParty/codemirror-5.52.0/src/model/chunk.js | 4 +--- ThirdParty/dojo-release-1.10.4/dojo/Evented.js | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/ThirdParty/codemirror-5.52.0/lib/codemirror.js b/ThirdParty/codemirror-5.52.0/lib/codemirror.js index 308a50dc97f0..9489e39c3a48 100644 --- a/ThirdParty/codemirror-5.52.0/lib/codemirror.js +++ b/ThirdParty/codemirror-5.52.0/lib/codemirror.js @@ -5604,9 +5604,7 @@ // Helper used to collapse a small branch into a single leaf. collapse: function(lines) { - for (let i = 0; i < this.lines.length; i++) { - lines.push(this.lines[i]); - } + lines.push.apply(lines, this.lines); }, // Insert the given array of lines at offset 'at', count them as diff --git a/ThirdParty/codemirror-5.52.0/src/model/chunk.js b/ThirdParty/codemirror-5.52.0/src/model/chunk.js index c4dbc8187a58..d82716ded4c1 100644 --- a/ThirdParty/codemirror-5.52.0/src/model/chunk.js +++ b/ThirdParty/codemirror-5.52.0/src/model/chunk.js @@ -42,9 +42,7 @@ LeafChunk.prototype = { // Helper used to collapse a small branch into a single leaf. collapse(lines) { - for (let i = 0; i < this.lines.length; ++i){ - lines.push(this.lines[i]) - } + lines.push.apply(lines, this.lines) }, // Insert the given array of lines at offset 'at', count them as diff --git a/ThirdParty/dojo-release-1.10.4/dojo/Evented.js b/ThirdParty/dojo-release-1.10.4/dojo/Evented.js index 0083a4f78ca1..208a2a231d71 100644 --- a/ThirdParty/dojo-release-1.10.4/dojo/Evented.js +++ b/ThirdParty/dojo-release-1.10.4/dojo/Evented.js @@ -28,9 +28,7 @@ define(["./aspect", "./on"], function(aspect, on){ }, emit: function(type, event){ var args = [this]; - for (var i = 0; i < arguments.length; i++){ - args.push(arguments[i]); - } + args.push.apply(args, arguments); return on.emit.apply(on, args); } }; From 1daf10591e69499db1bdfd31545a48d585365b41 Mon Sep 17 00:00:00 2001 From: Tim Quattrochi Date: Thu, 26 Sep 2024 14:26:31 -0400 Subject: [PATCH 6/6] refactor: made changes based on PR comments. --- .../Source/Scene/Cesium3DTileBatchTable.js | 13 ++++++++--- .../engine/Source/Scene/Cesium3DTileStyle.js | 18 ++++++++++----- packages/engine/Source/Scene/GltfLoader.js | 20 +++++----------- .../Scene/Model/InstancingPipelineStage.js | 6 +++-- packages/engine/Source/Scene/PropertyTable.js | 23 ++++++++++++++----- 5 files changed, 49 insertions(+), 31 deletions(-) diff --git a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js index f0ea897137b6..0f150cba341a 100644 --- a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js +++ b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js @@ -373,13 +373,20 @@ Cesium3DTileBatchTable.prototype.getPropertyIds = function (batchId, results) { results.length = 0; const scratchPropertyIds = Object.keys(this._properties); - for (let i = 0; i < scratchPropertyIds.length; ++i) { + const propertiesLength = scratchPropertyIds.length; + for (let i = 0; i < propertiesLength; ++i) { results.push(scratchPropertyIds[i]); } if (defined(this._batchTableHierarchy)) { - for (let i = 0; i < this._batchTableHierarchy._classes.length; ++i) { - results.push(this._batchTableHierarchy._classes[i].name); + const hierarchyPropertyIds = this._batchTableHierarchy.getPropertyIds( + batchId, + scratchPropertyIds + ); + + const hierarchyPropertyIdsLength = hierarchyPropertyIds.length; + for (let i = 0; i < hierarchyPropertyIdsLength; ++i) { + results.push(hierarchyPropertyIds[i]); } } diff --git a/packages/engine/Source/Scene/Cesium3DTileStyle.js b/packages/engine/Source/Scene/Cesium3DTileStyle.js index 92cbb8097b19..4280c4cae72e 100644 --- a/packages/engine/Source/Scene/Cesium3DTileStyle.js +++ b/packages/engine/Source/Scene/Cesium3DTileStyle.js @@ -1453,20 +1453,26 @@ Cesium3DTileStyle.prototype.getVariables = function () { let variables = []; if (defined(this.color) && defined(this.color.getVariables)) { - for (let i = 0; i < this.color.getVariables().length; i++) { - variables.push(this.color.getVariables()[i]); + const colorVariables = this.color.getVariables(); + const n = colorVariables.length; + for (let i = 0; i < n; i++) { + variables.push(colorVariables[i]); } } if (defined(this.show) && defined(this.show.getVariables)) { - for (let i = 0; i < this.show.getVariables().length; i++) { - variables.push(this.show.getVariables()[i]); + const showVariables = this.show.getVariables(); + const n = showVariables.length; + for (let i = 0; i < n; i++) { + variables.push(showVariables[i]); } } if (defined(this.pointSize) && defined(this.pointSize.getVariables)) { - for (let i = 0; i < this.pointSize.getVariables().length; i++) { - variables.push(this.pointSize.getVariables()[i]); + const pointSizeVariables = this.pointSize.getVariables(); + const n = pointSizeVariables.length; + for (let i = 0; i < n; i++) { + variables.push(pointSizeVariables[i]); } } diff --git a/packages/engine/Source/Scene/GltfLoader.js b/packages/engine/Source/Scene/GltfLoader.js index 0588f395f5a7..1ad97f29530f 100644 --- a/packages/engine/Source/Scene/GltfLoader.js +++ b/packages/engine/Source/Scene/GltfLoader.js @@ -2683,25 +2683,17 @@ function parse(loader, frameState) { // Gather promises and handle any errors const readyPromises = []; - for (let i = 0; i < loader._loaderPromises.length; ++i) { - const promise = loader._loaderPromises[i]; - promise.then(undefined, function (error) { - loader._state = GltfLoaderState.FAILED; - loader._error = error; - }); - readyPromises.push(promise); + const n = loader._loaderPromises.length; + for (let i = 0; i < n; ++i) { + readyPromises.push(loader._loaderPromises[i]); } // When incrementallyLoadTextures is true, the errors are caught and thrown individually // since it doesn't affect the overall loader state if (!loader._incrementallyLoadTextures) { - for (let i = 0; i < loader._textureLoaders.length; ++i) { - const promise = loader._textureLoaders[i].readyPromise; - promise.then(undefined, function (error) { - loader._state = GltfLoaderState.FAILED; - loader._error = error; - }); - readyPromises.push(promise); + const texturesPromisesLength = loader._texturesPromises.length; + for (let i = 0; i < texturesPromisesLength; ++i) { + readyPromises.push(loader._texturesPromises[i]); } } diff --git a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js index 5d4a0ff35dbb..8e7615b7efd3 100644 --- a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js +++ b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js @@ -210,8 +210,10 @@ InstancingPipelineStage.process = function (renderResources, node, frameState) { renderResources.uniformMap = combine(uniformMap, renderResources.uniformMap); renderResources.instanceCount = count; - for (let i = 0; i < instancingVertexAttributes.length; i++) { - instancingVertexAttributes[i].instanceCount = count; + + const instancingVertexAttributesLength = instancingVertexAttributes.length; + for (let i = 0; i < instancingVertexAttributesLength; i++) { + renderResources.attributes.push(instancingVertexAttributes[i]); } }; diff --git a/packages/engine/Source/Scene/PropertyTable.js b/packages/engine/Source/Scene/PropertyTable.js index 841bfa45903c..521b365ba59f 100644 --- a/packages/engine/Source/Scene/PropertyTable.js +++ b/packages/engine/Source/Scene/PropertyTable.js @@ -280,6 +280,8 @@ PropertyTable.prototype.propertyExistsBySemantic = function (semantic) { return false; }; +const scratchResults = []; + /** * Returns an array of property IDs. For compatibility with the 3DTILES_batch_table_hierarchy extension, this is computed for a specific feature. * @@ -294,20 +296,29 @@ PropertyTable.prototype.getPropertyIds = function (index, results) { if (defined(this._metadataTable)) { // concat in place to avoid unnecessary array allocation - for (let i = 0; i < this._metadataTable.propertyIds.length; i++) { - results.push(this._metadataTable.propertyIds[i]); + const propertyIds = this._metadataTable.getPropertyIds(scratchResults); + const n = propertyIds.length; + for (let i = 0; i < n; ++i) { + results.push(propertyIds[i]); } } if (defined(this._batchTableHierarchy)) { - for (let i = 0; i < this._batchTableHierarchy.propertyIds.length; i++) { - results.push(this._batchTableHierarchy.propertyIds[i]); + const propertyIds = this._batchTableHierarchy.getPropertyIds( + index, + scratchResults + ); + const n = propertyIds.length; + for (let i = 0; i < n; i++) { + results.push(propertyIds[i]); } } if (defined(this._jsonMetadataTable)) { - for (let i = 0; i < this._jsonMetadataTable.propertyIds.length; i++) { - results.push(this._jsonMetadataTable.propertyIds[i]); + const propertyIds = this._jsonMetadataTable.getPropertyIds(scratchResults); + const n = propertyIds.length; + for (let i = 0; i < n; i++) { + results.push(propertyIds[i]); } }