From f7455ad0271d23f5b3eba1314a41723fd403c3c0 Mon Sep 17 00:00:00 2001 From: Marco Hutter Date: Thu, 5 Dec 2024 17:29:15 +0100 Subject: [PATCH 1/5] Replace push.apply with dedicated function --- packages/engine/Source/Core/addAll.js | 41 +++++++++++++++++++ .../engine/Source/Renderer/ShaderBuilder.js | 13 +++--- .../Source/Scene/Cesium3DTileBatchTable.js | 10 +++-- .../engine/Source/Scene/Cesium3DTileStyle.js | 7 ++-- .../Source/Scene/ConditionsExpression.js | 5 ++- packages/engine/Source/Scene/GltfLoader.js | 5 ++- .../Source/Scene/MetadataTableProperty.js | 3 +- .../Model/ClassificationModelDrawCommand.js | 13 +++--- .../Source/Scene/Model/GeoJsonLoader.js | 4 +- .../Scene/Model/InstancingPipelineStage.js | 11 ++--- .../Source/Scene/Model/ModelSceneGraph.js | 3 +- packages/engine/Source/Scene/PropertyTable.js | 19 ++++----- packages/engine/Specs/Core/addAllSpec.js | 30 ++++++++++++++ .../ClassificationModelDrawCommandSpec.js | 5 ++- .../Model/ClassificationPipelineStageSpec.js | 3 +- .../Source/Animation/AnimationViewModel.js | 3 +- 16 files changed, 125 insertions(+), 50 deletions(-) create mode 100644 packages/engine/Source/Core/addAll.js create mode 100644 packages/engine/Specs/Core/addAllSpec.js diff --git a/packages/engine/Source/Core/addAll.js b/packages/engine/Source/Core/addAll.js new file mode 100644 index 000000000000..59401f0fd25d --- /dev/null +++ b/packages/engine/Source/Core/addAll.js @@ -0,0 +1,41 @@ +import defined from "./defined"; + +/** + * Adds all elements from the given source array to the given target array. + * + * If the `source` is `undefined`, then nothing will be done. Otherwise, + * this has the same semantics as + * ``` + * for (const s of source) target.push(s); + * ``` + * but is usually more efficient than a `for`-loop, and does not put the + * elements of the source on the stack, as it would be done with the + * spread operator or when using `target.push.apply(source)`. + * + * @function + * @private + * + * @param {Array|undefined} source The source array + * @param {Array} target The target array + * + * @example + * const target = [ 0, 1, 2 ]; + * const source = [ 3, 4, 5 ]; + * Cesium.addAll(source, target); + * // The target is now [ 0, 1, 2, 3, 4, 5 ] + */ +function addAll(source, target) { + if (!defined(source)) { + return; + } + const s = source.length; + if (s === 0) { + return; + } + const t = target.length; + target.length += s; + for (let i = 0; i < s; i++) { + target[t + i] = source[i]; + } +} +export default addAll; diff --git a/packages/engine/Source/Renderer/ShaderBuilder.js b/packages/engine/Source/Renderer/ShaderBuilder.js index 95fd920d5769..3bdbf8cfabbd 100644 --- a/packages/engine/Source/Renderer/ShaderBuilder.js +++ b/packages/engine/Source/Renderer/ShaderBuilder.js @@ -8,6 +8,7 @@ import ShaderProgram from "./ShaderProgram.js"; import ShaderSource from "./ShaderSource.js"; import ShaderStruct from "./ShaderStruct.js"; import ShaderFunction from "./ShaderFunction.js"; +import addAll from "../Core/addAll.js"; /** * An object that makes it easier to build the text of a {@link ShaderProgram}. This tracks GLSL code for both the vertex shader and the fragment shader. @@ -414,7 +415,7 @@ ShaderBuilder.prototype.addVertexLines = function (lines) { const vertexLines = this._vertexShaderParts.shaderLines; if (Array.isArray(lines)) { - vertexLines.push.apply(vertexLines, lines); + addAll(lines, vertexLines); } else { // Single string case vertexLines.push(lines); @@ -449,7 +450,7 @@ ShaderBuilder.prototype.addFragmentLines = function (lines) { const fragmentLines = this._fragmentShaderParts.shaderLines; if (Array.isArray(lines)) { - fragmentLines.push.apply(fragmentLines, lines); + addAll(lines, fragmentLines); } else { // Single string case fragmentLines.push(lines); @@ -534,7 +535,7 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - vertexLines.push.apply(vertexLines, structLines); + addAll(structLines, vertexLines); } structIds = shaderBuilder._fragmentShaderParts.structIds; @@ -542,7 +543,7 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - fragmentLines.push.apply(fragmentLines, structLines); + addAll(structLines, fragmentLines); } return { @@ -577,7 +578,7 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - vertexLines.push.apply(vertexLines, functionLines); + addAll(functionLines, vertexLines); } functionIds = shaderBuilder._fragmentShaderParts.functionIds; @@ -585,7 +586,7 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - fragmentLines.push.apply(fragmentLines, functionLines); + addAll(functionLines, fragmentLines); } return { diff --git a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js index 5bb9f4f3ac42..40d16215e37a 100644 --- a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js +++ b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js @@ -24,6 +24,7 @@ import getBinaryAccessor from "./getBinaryAccessor.js"; import StencilConstants from "./StencilConstants.js"; import StencilFunction from "./StencilFunction.js"; import StencilOperation from "./StencilOperation.js"; +import addAll from "../Core/addAll.js"; const DEFAULT_COLOR_VALUE = BatchTexture.DEFAULT_COLOR_VALUE; const DEFAULT_SHOW_VALUE = BatchTexture.DEFAULT_SHOW_VALUE; @@ -372,13 +373,14 @@ Cesium3DTileBatchTable.prototype.getPropertyIds = function (batchId, results) { results.length = 0; const scratchPropertyIds = Object.keys(this._properties); - results.push.apply(results, scratchPropertyIds); + addAll(scratchPropertyIds, results); if (defined(this._batchTableHierarchy)) { - results.push.apply( - results, - this._batchTableHierarchy.getPropertyIds(batchId, scratchPropertyIds), + const propertyIds = this._batchTableHierarchy.getPropertyIds( + batchId, + scratchPropertyIds, ); + addAll(propertyIds, results); } return results; diff --git a/packages/engine/Source/Scene/Cesium3DTileStyle.js b/packages/engine/Source/Scene/Cesium3DTileStyle.js index 5ebe368e10a0..4efaf167f103 100644 --- a/packages/engine/Source/Scene/Cesium3DTileStyle.js +++ b/packages/engine/Source/Scene/Cesium3DTileStyle.js @@ -1,3 +1,4 @@ +import addAll from "../Core/addAll.js"; import clone from "../Core/clone.js"; import defaultValue from "../Core/defaultValue.js"; import defined from "../Core/defined.js"; @@ -1455,15 +1456,15 @@ Cesium3DTileStyle.prototype.getVariables = function () { let variables = []; if (defined(this.color) && defined(this.color.getVariables)) { - variables.push.apply(variables, this.color.getVariables()); + addAll(this.color.getVariables(), variables); } if (defined(this.show) && defined(this.show.getVariables)) { - variables.push.apply(variables, this.show.getVariables()); + addAll(this.show.getVariables(), variables); } if (defined(this.pointSize) && defined(this.pointSize.getVariables)) { - variables.push.apply(variables, this.pointSize.getVariables()); + addAll(this.pointSize.getVariables(), variables); } // Remove duplicates diff --git a/packages/engine/Source/Scene/ConditionsExpression.js b/packages/engine/Source/Scene/ConditionsExpression.js index b88e01cc2053..44916b52ccc2 100644 --- a/packages/engine/Source/Scene/ConditionsExpression.js +++ b/packages/engine/Source/Scene/ConditionsExpression.js @@ -1,3 +1,4 @@ +import addAll from "../Core/addAll.js"; import clone from "../Core/clone.js"; import defined from "../Core/defined.js"; import Expression from "./Expression.js"; @@ -203,8 +204,8 @@ 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()); + addAll(statement.condition.getVariables(), variables); + addAll(statement.expression.getVariables(), variables); } // Remove duplicates diff --git a/packages/engine/Source/Scene/GltfLoader.js b/packages/engine/Source/Scene/GltfLoader.js index d853d90d73dc..8c213bb26c85 100644 --- a/packages/engine/Source/Scene/GltfLoader.js +++ b/packages/engine/Source/Scene/GltfLoader.js @@ -32,6 +32,7 @@ import VertexAttributeSemantic from "./VertexAttributeSemantic.js"; import GltfGpmLoader from "./Model/Extensions/Gpm/GltfGpmLoader.js"; import GltfMeshPrimitiveGpmLoader from "./Model/Extensions/Gpm/GltfMeshPrimitiveGpmLoader.js"; import oneTimeWarning from "../Core/oneTimeWarning.js"; +import addAll from "../Core/addAll.js"; const { Attribute, @@ -2749,12 +2750,12 @@ function parse(loader, frameState) { // Gather promises and handle any errors const readyPromises = []; - readyPromises.push.apply(readyPromises, loader._loaderPromises); + addAll(readyPromises, loader._loaderPromises); // 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); + addAll(readyPromises, loader._texturesPromises); } return Promise.all(readyPromises); diff --git a/packages/engine/Source/Scene/MetadataTableProperty.js b/packages/engine/Source/Scene/MetadataTableProperty.js index 3af58b4f559e..b927e5ea0f30 100644 --- a/packages/engine/Source/Scene/MetadataTableProperty.js +++ b/packages/engine/Source/Scene/MetadataTableProperty.js @@ -10,6 +10,7 @@ import oneTimeWarning from "../Core/oneTimeWarning.js"; import MetadataComponentType from "./MetadataComponentType.js"; import MetadataClassProperty from "./MetadataClassProperty.js"; import MetadataType from "./MetadataType.js"; +import addAll from "../Core/addAll.js"; /** * A binary property in a {@MetadataTable} @@ -388,7 +389,7 @@ function flatten(values) { for (let i = 0; i < values.length; i++) { const value = values[i]; if (Array.isArray(value)) { - result.push.apply(result, value); + addAll(value, result); } else { result.push(value); } diff --git a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js index 1b8c3c91115c..36506022b858 100644 --- a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js +++ b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js @@ -1,3 +1,4 @@ +import addAll from "../../Core/addAll.js"; import BoundingSphere from "../../Core/BoundingSphere.js"; import Check from "../../Core/Check.js"; import defaultValue from "../../Core/defaultValue.js"; @@ -482,16 +483,16 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( const passes = frameState.passes; if (passes.render) { if (this._useDebugWireframe) { - result.push.apply(result, this._commandListDebugWireframe); + addAll(this._commandListDebugWireframe, result); return; } if (this._classifiesTerrain) { - result.push.apply(result, this._commandListTerrain); + addAll(this._commandListTerrain, result); } if (this._classifies3DTiles) { - result.push.apply(result, this._commandList3DTiles); + addAll(this._commandList3DTiles, result); } const useIgnoreShowCommands = @@ -513,17 +514,17 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( ); } - result.push.apply(result, this._commandListIgnoreShow); + addAll(this._commandListIgnoreShow, result); } } if (passes.pick) { if (this._classifiesTerrain) { - result.push.apply(result, this._commandListTerrainPicking); + addAll(this._commandListTerrainPicking, result); } if (this._classifies3DTiles) { - result.push.apply(result, this._commandList3DTilesPicking); + addAll(this._commandList3DTilesPicking, result); } } diff --git a/packages/engine/Source/Scene/Model/GeoJsonLoader.js b/packages/engine/Source/Scene/Model/GeoJsonLoader.js index 092962cd10f5..2ba023532f0e 100644 --- a/packages/engine/Source/Scene/Model/GeoJsonLoader.js +++ b/packages/engine/Source/Scene/Model/GeoJsonLoader.js @@ -19,6 +19,7 @@ import StructuralMetadata from "../StructuralMetadata.js"; import VertexAttributeSemantic from "../VertexAttributeSemantic.js"; import Buffer from "../../Renderer/Buffer.js"; import BufferUsage from "../../Renderer/BufferUsage.js"; +import addAll from "../../Core/addAll.js"; /** * Loads a GeoJson model as part of the MAXAR_content_geojson extension with the following constraints: @@ -166,7 +167,8 @@ 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]); + addAll(polygon, lines); } return lines; } diff --git a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js index 682d2904cd5d..0347167084c1 100644 --- a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js +++ b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js @@ -1,3 +1,4 @@ +import addAll from "../../Core/addAll.js"; import AttributeCompression from "../../Core/AttributeCompression.js"; import Cartesian3 from "../../Core/Cartesian3.js"; import clone from "../../Core/clone.js"; @@ -210,10 +211,7 @@ InstancingPipelineStage.process = function (renderResources, node, frameState) { renderResources.uniformMap = combine(uniformMap, renderResources.uniformMap); renderResources.instanceCount = count; - renderResources.attributes.push.apply( - renderResources.attributes, - instancingVertexAttributes, - ); + addAll(instancingVertexAttributes, renderResources.attributes); }; const projectedTransformScratch = new Matrix4(); @@ -988,10 +986,7 @@ function processMatrixAttributes( shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row1`); shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row2`); - instancingVertexAttributes.push.apply( - instancingVertexAttributes, - matrixAttributes, - ); + addAll(matrixAttributes, instancingVertexAttributes); } function processVec3Attribute( diff --git a/packages/engine/Source/Scene/Model/ModelSceneGraph.js b/packages/engine/Source/Scene/Model/ModelSceneGraph.js index 248ee80f3403..e85290867535 100644 --- a/packages/engine/Source/Scene/Model/ModelSceneGraph.js +++ b/packages/engine/Source/Scene/Model/ModelSceneGraph.js @@ -26,6 +26,7 @@ import ModelType from "./ModelType.js"; import NodeRenderResources from "./NodeRenderResources.js"; import PrimitiveRenderResources from "./PrimitiveRenderResources.js"; import ModelDrawCommands from "./ModelDrawCommands.js"; +import addAll from "../../Core/addAll.js"; /** * An in memory representation of the scene graph for a {@link Model} @@ -912,7 +913,7 @@ ModelSceneGraph.prototype.pushDrawCommands = function (frameState) { pushDrawCommandOptions, ); - frameState.commandList.push.apply(frameState.commandList, silhouetteCommands); + addAll(silhouetteCommands, frameState.commandList); }; // 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 cbb73b3cfd9b..79b81053d0c8 100644 --- a/packages/engine/Source/Scene/PropertyTable.js +++ b/packages/engine/Source/Scene/PropertyTable.js @@ -3,6 +3,7 @@ import defaultValue from "../Core/defaultValue.js"; import DeveloperError from "../Core/DeveloperError.js"; import defined from "../Core/defined.js"; import JsonMetadataTable from "./JsonMetadataTable.js"; +import addAll from "../Core/addAll.js"; /** * A property table for use with the EXT_structural_metadata extension or @@ -296,24 +297,18 @@ 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), - ); + const ids = this._metadataTable.getPropertyIds(scratchResults); + addAll(ids, results); } if (defined(this._batchTableHierarchy)) { - results.push.apply( - results, - this._batchTableHierarchy.getPropertyIds(index, scratchResults), - ); + const ids = this._batchTableHierarchy.getPropertyIds(index, scratchResults); + addAll(ids, results); } if (defined(this._jsonMetadataTable)) { - results.push.apply( - results, - this._jsonMetadataTable.getPropertyIds(scratchResults), - ); + const ids = this._jsonMetadataTable.getPropertyIds(scratchResults); + addAll(ids, results); } return results; diff --git a/packages/engine/Specs/Core/addAllSpec.js b/packages/engine/Specs/Core/addAllSpec.js new file mode 100644 index 000000000000..da6755cc0076 --- /dev/null +++ b/packages/engine/Specs/Core/addAllSpec.js @@ -0,0 +1,30 @@ +import { addAll } from "../../index.js"; + +describe("Core/addAll", function () { + it("works for basic arrays", function () { + const source = [3, 4, 5]; + const target = [0, 1, 2]; + addAll(source, target); + expect(target).toEqual([0, 1, 2, 3, 4, 5]); + }); + + it("works for null source", function () { + const target = [0, 1, 2]; + addAll(null, target); + expect(target).toEqual([0, 1, 2]); + }); + + it("works for undefined source", function () { + const target = [0, 1, 2]; + addAll(undefined, target); + expect(target).toEqual([0, 1, 2]); + }); + + it("works for large arrays", function () { + const source = Array(200000); + const target = Array(200000); + const result = Array(400000); + addAll(source, target); + expect(target).toEqual(result); + }); +}); diff --git a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js index adfa295c7948..1abf3ffc83df 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js @@ -1,4 +1,5 @@ import { + addAll, BlendingState, BoundingSphere, Cartesian3, @@ -814,8 +815,8 @@ describe( expect(drawCommand.modelMatrix).toBe(command.modelMatrix); const commandList = []; - commandList.push.apply(commandList, drawCommand._commandListTerrain); - commandList.push.apply(commandList, drawCommand._commandList3DTiles); + addAll(drawCommand._commandListTerrain, commandList); + addAll(drawCommand._commandList3DTiles, commandList); 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 d62b7184c13c..059fc7df5d2f 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js @@ -1,4 +1,5 @@ import { + addAll, ClassificationPipelineStage, PrimitiveType, RuntimeError, @@ -19,7 +20,7 @@ describe("Scene/Model/ClassificationPipelineStage", function () { const batchLength = batchLengths[id]; const batch = new Array(batchLength); batch.fill(id); - featureIds.push.apply(featureIds, batch); + addAll(batch, featureIds); } return featureIds; diff --git a/packages/widgets/Source/Animation/AnimationViewModel.js b/packages/widgets/Source/Animation/AnimationViewModel.js index f00e762cd677..d30a47f7e432 100644 --- a/packages/widgets/Source/Animation/AnimationViewModel.js +++ b/packages/widgets/Source/Animation/AnimationViewModel.js @@ -1,4 +1,5 @@ import { + addAll, binarySearch, ClockRange, ClockStep, @@ -506,7 +507,7 @@ AnimationViewModel.prototype.setShuttleRingTicks = function (positiveTicks) { allTicks.push(-tick); } } - Array.prototype.push.apply(allTicks, sortedFilteredPositiveTicks); + addAll(sortedFilteredPositiveTicks, allTicks); this._allShuttleRingTicks = allTicks; }; From c088b639bb16b7d73d2222ba15c2b539458b7c69 Mon Sep 17 00:00:00 2001 From: Marco Hutter Date: Thu, 5 Dec 2024 18:05:07 +0100 Subject: [PATCH 2/5] Proper import of defined.js --- packages/engine/Source/Core/addAll.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/engine/Source/Core/addAll.js b/packages/engine/Source/Core/addAll.js index 59401f0fd25d..c2e0cfbc6d69 100644 --- a/packages/engine/Source/Core/addAll.js +++ b/packages/engine/Source/Core/addAll.js @@ -1,4 +1,4 @@ -import defined from "./defined"; +import defined from "./defined.js"; /** * Adds all elements from the given source array to the given target array. From c00243a948e5a8dd99a0fe6bdd5d91d5d60bbbb2 Mon Sep 17 00:00:00 2001 From: Marco Hutter Date: Thu, 5 Dec 2024 18:40:01 +0100 Subject: [PATCH 3/5] Fix argument order --- packages/engine/Source/Scene/GltfLoader.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/engine/Source/Scene/GltfLoader.js b/packages/engine/Source/Scene/GltfLoader.js index 8c213bb26c85..6a54a2d986e3 100644 --- a/packages/engine/Source/Scene/GltfLoader.js +++ b/packages/engine/Source/Scene/GltfLoader.js @@ -2750,12 +2750,12 @@ function parse(loader, frameState) { // Gather promises and handle any errors const readyPromises = []; - addAll(readyPromises, loader._loaderPromises); + addAll(loader._loaderPromises, readyPromises); // When incrementallyLoadTextures is true, the errors are caught and thrown individually // since it doesn't affect the overall loader state if (!loader._incrementallyLoadTextures) { - addAll(readyPromises, loader._texturesPromises); + addAll(loader._texturesPromises, readyPromises); } return Promise.all(readyPromises); From 62c488878e5cb33475202e31cb5487ab3cf12256 Mon Sep 17 00:00:00 2001 From: Marco Hutter Date: Wed, 11 Dec 2024 23:58:21 +0100 Subject: [PATCH 4/5] Update JSDoc Co-authored-by: jjspace <8007967+jjspace@users.noreply.github.com> --- packages/engine/Source/Core/addAll.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/engine/Source/Core/addAll.js b/packages/engine/Source/Core/addAll.js index c2e0cfbc6d69..c4e828775ad4 100644 --- a/packages/engine/Source/Core/addAll.js +++ b/packages/engine/Source/Core/addAll.js @@ -3,7 +3,7 @@ import defined from "./defined.js"; /** * Adds all elements from the given source array to the given target array. * - * If the `source` is `undefined`, then nothing will be done. Otherwise, + * If the source is undefined, then nothing will be done. Otherwise, * this has the same semantics as * ``` * for (const s of source) target.push(s); From 637288044b1c0c844f0bb13ad8f72b5fc8e8bb64 Mon Sep 17 00:00:00 2001 From: Marco Hutter Date: Fri, 13 Dec 2024 15:27:46 +0100 Subject: [PATCH 5/5] Change addAll to addAllToArray, based on review --- packages/engine/Source/Core/addAll.js | 41 ------------------- packages/engine/Source/Core/addAllToArray.js | 40 ++++++++++++++++++ .../engine/Source/Renderer/ShaderBuilder.js | 14 +++---- .../Source/Scene/Cesium3DTileBatchTable.js | 6 +-- .../engine/Source/Scene/Cesium3DTileStyle.js | 8 ++-- .../Source/Scene/ConditionsExpression.js | 6 +-- packages/engine/Source/Scene/GltfLoader.js | 6 +-- .../Source/Scene/MetadataTableProperty.js | 4 +- .../Model/ClassificationModelDrawCommand.js | 14 +++---- .../Source/Scene/Model/GeoJsonLoader.js | 4 +- .../Scene/Model/InstancingPipelineStage.js | 6 +-- .../Source/Scene/Model/ModelSceneGraph.js | 4 +- packages/engine/Source/Scene/PropertyTable.js | 8 ++-- packages/engine/Specs/Core/addAllSpec.js | 12 +++--- .../ClassificationModelDrawCommandSpec.js | 6 +-- .../Model/ClassificationPipelineStageSpec.js | 4 +- .../Source/Animation/AnimationViewModel.js | 4 +- 17 files changed, 93 insertions(+), 94 deletions(-) delete mode 100644 packages/engine/Source/Core/addAll.js create mode 100644 packages/engine/Source/Core/addAllToArray.js diff --git a/packages/engine/Source/Core/addAll.js b/packages/engine/Source/Core/addAll.js deleted file mode 100644 index c4e828775ad4..000000000000 --- a/packages/engine/Source/Core/addAll.js +++ /dev/null @@ -1,41 +0,0 @@ -import defined from "./defined.js"; - -/** - * Adds all elements from the given source array to the given target array. - * - * If the source is undefined, then nothing will be done. Otherwise, - * this has the same semantics as - * ``` - * for (const s of source) target.push(s); - * ``` - * but is usually more efficient than a `for`-loop, and does not put the - * elements of the source on the stack, as it would be done with the - * spread operator or when using `target.push.apply(source)`. - * - * @function - * @private - * - * @param {Array|undefined} source The source array - * @param {Array} target The target array - * - * @example - * const target = [ 0, 1, 2 ]; - * const source = [ 3, 4, 5 ]; - * Cesium.addAll(source, target); - * // The target is now [ 0, 1, 2, 3, 4, 5 ] - */ -function addAll(source, target) { - if (!defined(source)) { - return; - } - const s = source.length; - if (s === 0) { - return; - } - const t = target.length; - target.length += s; - for (let i = 0; i < s; i++) { - target[t + i] = source[i]; - } -} -export default addAll; diff --git a/packages/engine/Source/Core/addAllToArray.js b/packages/engine/Source/Core/addAllToArray.js new file mode 100644 index 000000000000..929e97fe0d30 --- /dev/null +++ b/packages/engine/Source/Core/addAllToArray.js @@ -0,0 +1,40 @@ +import defined from "./defined.js"; + +/** + * Adds all elements from the given source array to the given target array. + * + * If the source is null, undefined, + * or empty, then nothing will be done. Otherwise, this has the same + * semantics as
+ * for (const s of source) target.push(s);
+ * but is usually more efficient than a for-loop, and does not + * put the elements of the source on the stack, as it would be done with the + * spread operator or when using target.push.apply(source). + * + * @function + * @private + * + * @param {Array} target The target array + * @param {Array|undefined} source The source array + * + * @example + * const target = [ 0, 1, 2 ]; + * const source = [ 3, 4, 5 ]; + * Cesium.addAllToArray(target, source); + * // The target is now [ 0, 1, 2, 3, 4, 5 ] + */ +function addAllToArray(target, source) { + if (!defined(source)) { + return; + } + const sourceLength = source.length; + if (sourceLength === 0) { + return; + } + const targetLength = target.length; + target.length += sourceLength; + for (let i = 0; i < sourceLength; i++) { + target[targetLength + i] = source[i]; + } +} +export default addAllToArray; diff --git a/packages/engine/Source/Renderer/ShaderBuilder.js b/packages/engine/Source/Renderer/ShaderBuilder.js index 3bdbf8cfabbd..48fb8cb2267a 100644 --- a/packages/engine/Source/Renderer/ShaderBuilder.js +++ b/packages/engine/Source/Renderer/ShaderBuilder.js @@ -8,7 +8,7 @@ import ShaderProgram from "./ShaderProgram.js"; import ShaderSource from "./ShaderSource.js"; import ShaderStruct from "./ShaderStruct.js"; import ShaderFunction from "./ShaderFunction.js"; -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; /** * An object that makes it easier to build the text of a {@link ShaderProgram}. This tracks GLSL code for both the vertex shader and the fragment shader. @@ -415,7 +415,7 @@ ShaderBuilder.prototype.addVertexLines = function (lines) { const vertexLines = this._vertexShaderParts.shaderLines; if (Array.isArray(lines)) { - addAll(lines, vertexLines); + addAllToArray(vertexLines, lines); } else { // Single string case vertexLines.push(lines); @@ -450,7 +450,7 @@ ShaderBuilder.prototype.addFragmentLines = function (lines) { const fragmentLines = this._fragmentShaderParts.shaderLines; if (Array.isArray(lines)) { - addAll(lines, fragmentLines); + addAllToArray(fragmentLines, lines); } else { // Single string case fragmentLines.push(lines); @@ -535,7 +535,7 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - addAll(structLines, vertexLines); + addAllToArray(vertexLines, structLines); } structIds = shaderBuilder._fragmentShaderParts.structIds; @@ -543,7 +543,7 @@ function generateStructLines(shaderBuilder) { structId = structIds[i]; struct = shaderBuilder._structs[structId]; structLines = struct.generateGlslLines(); - addAll(structLines, fragmentLines); + addAllToArray(fragmentLines, structLines); } return { @@ -578,7 +578,7 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - addAll(functionLines, vertexLines); + addAllToArray(vertexLines, functionLines); } functionIds = shaderBuilder._fragmentShaderParts.functionIds; @@ -586,7 +586,7 @@ function generateFunctionLines(shaderBuilder) { functionId = functionIds[i]; func = shaderBuilder._functions[functionId]; functionLines = func.generateGlslLines(); - addAll(functionLines, fragmentLines); + addAllToArray(fragmentLines, functionLines); } return { diff --git a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js index 40d16215e37a..4b758ea20ec5 100644 --- a/packages/engine/Source/Scene/Cesium3DTileBatchTable.js +++ b/packages/engine/Source/Scene/Cesium3DTileBatchTable.js @@ -24,7 +24,7 @@ import getBinaryAccessor from "./getBinaryAccessor.js"; import StencilConstants from "./StencilConstants.js"; import StencilFunction from "./StencilFunction.js"; import StencilOperation from "./StencilOperation.js"; -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; const DEFAULT_COLOR_VALUE = BatchTexture.DEFAULT_COLOR_VALUE; const DEFAULT_SHOW_VALUE = BatchTexture.DEFAULT_SHOW_VALUE; @@ -373,14 +373,14 @@ Cesium3DTileBatchTable.prototype.getPropertyIds = function (batchId, results) { results.length = 0; const scratchPropertyIds = Object.keys(this._properties); - addAll(scratchPropertyIds, results); + addAllToArray(results, scratchPropertyIds); if (defined(this._batchTableHierarchy)) { const propertyIds = this._batchTableHierarchy.getPropertyIds( batchId, scratchPropertyIds, ); - addAll(propertyIds, results); + addAllToArray(results, propertyIds); } return results; diff --git a/packages/engine/Source/Scene/Cesium3DTileStyle.js b/packages/engine/Source/Scene/Cesium3DTileStyle.js index 4efaf167f103..28d5f84323bf 100644 --- a/packages/engine/Source/Scene/Cesium3DTileStyle.js +++ b/packages/engine/Source/Scene/Cesium3DTileStyle.js @@ -1,4 +1,4 @@ -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; import clone from "../Core/clone.js"; import defaultValue from "../Core/defaultValue.js"; import defined from "../Core/defined.js"; @@ -1456,15 +1456,15 @@ Cesium3DTileStyle.prototype.getVariables = function () { let variables = []; if (defined(this.color) && defined(this.color.getVariables)) { - addAll(this.color.getVariables(), variables); + addAllToArray(variables, this.color.getVariables()); } if (defined(this.show) && defined(this.show.getVariables)) { - addAll(this.show.getVariables(), variables); + addAllToArray(variables, this.show.getVariables()); } if (defined(this.pointSize) && defined(this.pointSize.getVariables)) { - addAll(this.pointSize.getVariables(), variables); + addAllToArray(variables, this.pointSize.getVariables()); } // Remove duplicates diff --git a/packages/engine/Source/Scene/ConditionsExpression.js b/packages/engine/Source/Scene/ConditionsExpression.js index 44916b52ccc2..f2c16330d741 100644 --- a/packages/engine/Source/Scene/ConditionsExpression.js +++ b/packages/engine/Source/Scene/ConditionsExpression.js @@ -1,4 +1,4 @@ -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; import clone from "../Core/clone.js"; import defined from "../Core/defined.js"; import Expression from "./Expression.js"; @@ -204,8 +204,8 @@ ConditionsExpression.prototype.getVariables = function () { const length = conditions.length; for (let i = 0; i < length; ++i) { const statement = conditions[i]; - addAll(statement.condition.getVariables(), variables); - addAll(statement.expression.getVariables(), variables); + addAllToArray(variables, statement.condition.getVariables()); + addAllToArray(variables, statement.expression.getVariables()); } // Remove duplicates diff --git a/packages/engine/Source/Scene/GltfLoader.js b/packages/engine/Source/Scene/GltfLoader.js index 6a54a2d986e3..4d2d8cd37a93 100644 --- a/packages/engine/Source/Scene/GltfLoader.js +++ b/packages/engine/Source/Scene/GltfLoader.js @@ -32,7 +32,7 @@ import VertexAttributeSemantic from "./VertexAttributeSemantic.js"; import GltfGpmLoader from "./Model/Extensions/Gpm/GltfGpmLoader.js"; import GltfMeshPrimitiveGpmLoader from "./Model/Extensions/Gpm/GltfMeshPrimitiveGpmLoader.js"; import oneTimeWarning from "../Core/oneTimeWarning.js"; -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; const { Attribute, @@ -2750,12 +2750,12 @@ function parse(loader, frameState) { // Gather promises and handle any errors const readyPromises = []; - addAll(loader._loaderPromises, readyPromises); + addAllToArray(readyPromises, loader._loaderPromises); // When incrementallyLoadTextures is true, the errors are caught and thrown individually // since it doesn't affect the overall loader state if (!loader._incrementallyLoadTextures) { - addAll(loader._texturesPromises, readyPromises); + addAllToArray(readyPromises, loader._texturesPromises); } return Promise.all(readyPromises); diff --git a/packages/engine/Source/Scene/MetadataTableProperty.js b/packages/engine/Source/Scene/MetadataTableProperty.js index b927e5ea0f30..24dcf94fdb71 100644 --- a/packages/engine/Source/Scene/MetadataTableProperty.js +++ b/packages/engine/Source/Scene/MetadataTableProperty.js @@ -10,7 +10,7 @@ import oneTimeWarning from "../Core/oneTimeWarning.js"; import MetadataComponentType from "./MetadataComponentType.js"; import MetadataClassProperty from "./MetadataClassProperty.js"; import MetadataType from "./MetadataType.js"; -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; /** * A binary property in a {@MetadataTable} @@ -389,7 +389,7 @@ function flatten(values) { for (let i = 0; i < values.length; i++) { const value = values[i]; if (Array.isArray(value)) { - addAll(value, result); + addAllToArray(result, value); } else { result.push(value); } diff --git a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js index 36506022b858..ea051f0e4752 100644 --- a/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js +++ b/packages/engine/Source/Scene/Model/ClassificationModelDrawCommand.js @@ -1,4 +1,4 @@ -import addAll from "../../Core/addAll.js"; +import addAllToArray from "../../Core/addAllToArray.js"; import BoundingSphere from "../../Core/BoundingSphere.js"; import Check from "../../Core/Check.js"; import defaultValue from "../../Core/defaultValue.js"; @@ -483,16 +483,16 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( const passes = frameState.passes; if (passes.render) { if (this._useDebugWireframe) { - addAll(this._commandListDebugWireframe, result); + addAllToArray(result, this._commandListDebugWireframe); return; } if (this._classifiesTerrain) { - addAll(this._commandListTerrain, result); + addAllToArray(result, this._commandListTerrain); } if (this._classifies3DTiles) { - addAll(this._commandList3DTiles, result); + addAllToArray(result, this._commandList3DTiles); } const useIgnoreShowCommands = @@ -514,17 +514,17 @@ ClassificationModelDrawCommand.prototype.pushCommands = function ( ); } - addAll(this._commandListIgnoreShow, result); + addAllToArray(result, this._commandListIgnoreShow); } } if (passes.pick) { if (this._classifiesTerrain) { - addAll(this._commandListTerrainPicking, result); + addAllToArray(result, this._commandListTerrainPicking); } if (this._classifies3DTiles) { - addAll(this._commandList3DTilesPicking, result); + addAllToArray(result, this._commandList3DTilesPicking); } } diff --git a/packages/engine/Source/Scene/Model/GeoJsonLoader.js b/packages/engine/Source/Scene/Model/GeoJsonLoader.js index 2ba023532f0e..17b1e4cf1a3f 100644 --- a/packages/engine/Source/Scene/Model/GeoJsonLoader.js +++ b/packages/engine/Source/Scene/Model/GeoJsonLoader.js @@ -19,7 +19,7 @@ import StructuralMetadata from "../StructuralMetadata.js"; import VertexAttributeSemantic from "../VertexAttributeSemantic.js"; import Buffer from "../../Renderer/Buffer.js"; import BufferUsage from "../../Renderer/BufferUsage.js"; -import addAll from "../../Core/addAll.js"; +import addAllToArray from "../../Core/addAllToArray.js"; /** * Loads a GeoJson model as part of the MAXAR_content_geojson extension with the following constraints: @@ -168,7 +168,7 @@ function parseMultiPolygon(coordinates) { const lines = []; for (let i = 0; i < polygonsLength; i++) { const polygon = parsePolygon(coordinates[i]); - addAll(polygon, lines); + addAllToArray(lines, polygon); } return lines; } diff --git a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js index 0347167084c1..620f49016080 100644 --- a/packages/engine/Source/Scene/Model/InstancingPipelineStage.js +++ b/packages/engine/Source/Scene/Model/InstancingPipelineStage.js @@ -1,4 +1,4 @@ -import addAll from "../../Core/addAll.js"; +import addAllToArray from "../../Core/addAllToArray.js"; import AttributeCompression from "../../Core/AttributeCompression.js"; import Cartesian3 from "../../Core/Cartesian3.js"; import clone from "../../Core/clone.js"; @@ -211,7 +211,7 @@ InstancingPipelineStage.process = function (renderResources, node, frameState) { renderResources.uniformMap = combine(uniformMap, renderResources.uniformMap); renderResources.instanceCount = count; - addAll(instancingVertexAttributes, renderResources.attributes); + addAllToArray(renderResources.attributes, instancingVertexAttributes); }; const projectedTransformScratch = new Matrix4(); @@ -986,7 +986,7 @@ function processMatrixAttributes( shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row1`); shaderBuilder.addAttribute("vec4", `a_instancing${attributeString}Row2`); - addAll(matrixAttributes, instancingVertexAttributes); + addAllToArray(instancingVertexAttributes, matrixAttributes); } function processVec3Attribute( diff --git a/packages/engine/Source/Scene/Model/ModelSceneGraph.js b/packages/engine/Source/Scene/Model/ModelSceneGraph.js index e85290867535..25e35fb62a60 100644 --- a/packages/engine/Source/Scene/Model/ModelSceneGraph.js +++ b/packages/engine/Source/Scene/Model/ModelSceneGraph.js @@ -26,7 +26,7 @@ import ModelType from "./ModelType.js"; import NodeRenderResources from "./NodeRenderResources.js"; import PrimitiveRenderResources from "./PrimitiveRenderResources.js"; import ModelDrawCommands from "./ModelDrawCommands.js"; -import addAll from "../../Core/addAll.js"; +import addAllToArray from "../../Core/addAllToArray.js"; /** * An in memory representation of the scene graph for a {@link Model} @@ -913,7 +913,7 @@ ModelSceneGraph.prototype.pushDrawCommands = function (frameState) { pushDrawCommandOptions, ); - addAll(silhouetteCommands, frameState.commandList); + addAllToArray(frameState.commandList, silhouetteCommands); }; // 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 79b81053d0c8..35b1b1e20a2b 100644 --- a/packages/engine/Source/Scene/PropertyTable.js +++ b/packages/engine/Source/Scene/PropertyTable.js @@ -3,7 +3,7 @@ import defaultValue from "../Core/defaultValue.js"; import DeveloperError from "../Core/DeveloperError.js"; import defined from "../Core/defined.js"; import JsonMetadataTable from "./JsonMetadataTable.js"; -import addAll from "../Core/addAll.js"; +import addAllToArray from "../Core/addAllToArray.js"; /** * A property table for use with the EXT_structural_metadata extension or @@ -298,17 +298,17 @@ PropertyTable.prototype.getPropertyIds = function (index, results) { if (defined(this._metadataTable)) { // concat in place to avoid unnecessary array allocation const ids = this._metadataTable.getPropertyIds(scratchResults); - addAll(ids, results); + addAllToArray(results, ids); } if (defined(this._batchTableHierarchy)) { const ids = this._batchTableHierarchy.getPropertyIds(index, scratchResults); - addAll(ids, results); + addAllToArray(results, ids); } if (defined(this._jsonMetadataTable)) { const ids = this._jsonMetadataTable.getPropertyIds(scratchResults); - addAll(ids, results); + addAllToArray(results, ids); } return results; diff --git a/packages/engine/Specs/Core/addAllSpec.js b/packages/engine/Specs/Core/addAllSpec.js index da6755cc0076..15aa3e3f39b4 100644 --- a/packages/engine/Specs/Core/addAllSpec.js +++ b/packages/engine/Specs/Core/addAllSpec.js @@ -1,22 +1,22 @@ -import { addAll } from "../../index.js"; +import { addAllToArray } from "../../index.js"; -describe("Core/addAll", function () { +describe("Core/addAllToArray", function () { it("works for basic arrays", function () { const source = [3, 4, 5]; const target = [0, 1, 2]; - addAll(source, target); + addAllToArray(target, source); expect(target).toEqual([0, 1, 2, 3, 4, 5]); }); it("works for null source", function () { const target = [0, 1, 2]; - addAll(null, target); + addAllToArray(target, null); expect(target).toEqual([0, 1, 2]); }); it("works for undefined source", function () { const target = [0, 1, 2]; - addAll(undefined, target); + addAllToArray(target, undefined); expect(target).toEqual([0, 1, 2]); }); @@ -24,7 +24,7 @@ describe("Core/addAll", function () { const source = Array(200000); const target = Array(200000); const result = Array(400000); - addAll(source, target); + addAllToArray(target, source); expect(target).toEqual(result); }); }); diff --git a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js index 1abf3ffc83df..607076df9338 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationModelDrawCommandSpec.js @@ -1,5 +1,5 @@ import { - addAll, + addAllToArray, BlendingState, BoundingSphere, Cartesian3, @@ -815,8 +815,8 @@ describe( expect(drawCommand.modelMatrix).toBe(command.modelMatrix); const commandList = []; - addAll(drawCommand._commandListTerrain, commandList); - addAll(drawCommand._commandList3DTiles, commandList); + addAllToArray(commandList, drawCommand._commandListTerrain); + addAllToArray(commandList, drawCommand._commandList3DTiles); 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 059fc7df5d2f..c816f0fee252 100644 --- a/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js +++ b/packages/engine/Specs/Scene/Model/ClassificationPipelineStageSpec.js @@ -1,5 +1,5 @@ import { - addAll, + addAllToArray, ClassificationPipelineStage, PrimitiveType, RuntimeError, @@ -20,7 +20,7 @@ describe("Scene/Model/ClassificationPipelineStage", function () { const batchLength = batchLengths[id]; const batch = new Array(batchLength); batch.fill(id); - addAll(batch, featureIds); + addAllToArray(featureIds, batch); } return featureIds; diff --git a/packages/widgets/Source/Animation/AnimationViewModel.js b/packages/widgets/Source/Animation/AnimationViewModel.js index d30a47f7e432..b74d0ba536eb 100644 --- a/packages/widgets/Source/Animation/AnimationViewModel.js +++ b/packages/widgets/Source/Animation/AnimationViewModel.js @@ -1,5 +1,5 @@ import { - addAll, + addAllToArray, binarySearch, ClockRange, ClockStep, @@ -507,7 +507,7 @@ AnimationViewModel.prototype.setShuttleRingTicks = function (positiveTicks) { allTicks.push(-tick); } } - addAll(sortedFilteredPositiveTicks, allTicks); + addAllToArray(allTicks, sortedFilteredPositiveTicks); this._allShuttleRingTicks = allTicks; };