Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 12053- CesiumJS should not use push.apply #12176

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
4 changes: 3 additions & 1 deletion ThirdParty/codemirror-5.52.0/lib/codemirror.js
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
lines.push(this.lines[i]);
}
},

// Insert the given array of lines at offset 'at', count them as
Expand Down
4 changes: 3 additions & 1 deletion ThirdParty/codemirror-5.52.0/src/model/chunk.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion ThirdParty/dojo-release-1.10.4/dojo/Evented.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
Expand Down
24 changes: 18 additions & 6 deletions packages/engine/Source/Renderer/ShaderBuilder.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -534,15 +538,19 @@ 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;
for (i = 0; i < structIds.length; i++) {
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 {
Expand Down Expand Up @@ -577,15 +585,19 @@ 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;
for (i = 0; i < functionIds.length; i++) {
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 {
Expand Down
11 changes: 6 additions & 5 deletions packages/engine/Source/Scene/Cesium3DTileBatchTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
}

return results;
Expand Down
12 changes: 9 additions & 3 deletions packages/engine/Source/Scene/Cesium3DTileStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
8 changes: 6 additions & 2 deletions packages/engine/Source/Scene/ConditionsExpression.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 16 additions & 2 deletions packages/engine/Source/Scene/GltfLoader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
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);
Expand Down
19 changes: 9 additions & 10 deletions packages/engine/Source/Scene/MetadataTableProperty.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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

Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
let value = BigInt(0);
const isNegative = (dataView.getUint8(byteOffset + 7) & 0x80) > 0;
let carrying = true;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -783,15 +784,14 @@ function BufferView(bufferView, componentType, length) {
return getInt64BigIntFallback(index, that);
};
} else {
// eslint-disable-next-line
typedArray = new BigInt64Array(
bufferView.buffer,
bufferView.byteOffset,
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) {
Expand All @@ -817,15 +817,14 @@ function BufferView(bufferView, componentType, length) {
return getUint64BigIntFallback(index, that);
};
} else {
// eslint-disable-next-line
typedArray = new BigUint64Array(
bufferView.buffer,
bufferView.byteOffset,
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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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]);
}
}
}

Expand Down
6 changes: 5 additions & 1 deletion packages/engine/Source/Scene/Model/GeoJsonLoader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
14 changes: 6 additions & 8 deletions packages/engine/Source/Scene/Model/InstancingPipelineStage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Tim-Quattrochi marked this conversation as resolved.
Show resolved Hide resolved
}
};

const projectedTransformScratch = new Matrix4();
Expand Down Expand Up @@ -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(
Expand Down
5 changes: 4 additions & 1 deletion packages/engine/Source/Scene/Model/ModelSceneGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading