From da441136b16406ac474edd28c2f5eb6f56875ba5 Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Tue, 21 Nov 2023 00:10:33 -0500 Subject: [PATCH 01/11] Remove destroyed PolylineCollection from Scene #7758 Remove PolylineCollections from the Scene before they are destroyed. This causes #7758 and #9154. --- .../Source/DataSources/PolylineGeometryUpdater.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js index 6ad349c9d862..6c3a595a9aa5 100644 --- a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js +++ b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js @@ -874,6 +874,7 @@ DynamicGeometryUpdater.prototype.destroy = function () { if (defined(polylineCollection)) { polylineCollection.remove(this._line); if (polylineCollection.length === 0) { + removePolylineCollection(geometryUpdater._scene.primitives, polylineCollection); this._primitives.removeAndDestroy(polylineCollection); delete polylineCollections[sceneId]; } @@ -883,4 +884,16 @@ DynamicGeometryUpdater.prototype.destroy = function () { } destroyObject(this); }; + +function removePolylineCollection(primitiveCollection, toRemove) { + if (defined(primitiveCollection)) { + if (typeof primitiveCollection.remove === "function") { + primitiveCollection.remove(toRemove); + } + + for (let i = 0; i < primitiveCollection.length; i++) { + removePolylineCollection(primitiveCollection.get(i), toRemove); + } + } +}; export default PolylineGeometryUpdater; From ae5202c652a0ce1c6f6d34f1b7e6528b33bf7055 Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Tue, 21 Nov 2023 12:56:12 -0500 Subject: [PATCH 02/11] Do not destroy primitives that are already destroyed --- .../engine/Source/DataSources/EntityCluster.js | 15 +++++++++++++++ .../engine/Source/Scene/PrimitiveCollection.js | 4 ++-- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/engine/Source/DataSources/EntityCluster.js b/packages/engine/Source/DataSources/EntityCluster.js index 965e19f74a08..baf2156d6ef5 100644 --- a/packages/engine/Source/DataSources/EntityCluster.js +++ b/packages/engine/Source/DataSources/EntityCluster.js @@ -921,6 +921,20 @@ EntityCluster.prototype.update = function (frameState) { } }; +/** + * Returns true if this object was destroyed; otherwise, false. + *

+ * If this object was destroyed, it should not be used; calling any function other than + * isDestroyed will result in a {@link DeveloperError} exception. + * + * @returns {boolean} True if this object was destroyed; otherwise, false. + * + * @see EntityCluster#destroy + */ +EntityCluster.prototype.isDestroyed = function () { + return false; +}; + /** * Destroys the WebGL resources held by this object. Destroying an object allows for deterministic * release of WebGL resources, instead of relying on the garbage collector to destroy this object. @@ -928,6 +942,7 @@ EntityCluster.prototype.update = function (frameState) { * Unlike other objects that use WebGL resources, this object can be reused. For example, if a data source is removed * from a data source collection and added to another. *

+ * As a result, this object is not destroyed in this function. */ EntityCluster.prototype.destroy = function () { this._labelCollection = diff --git a/packages/engine/Source/Scene/PrimitiveCollection.js b/packages/engine/Source/Scene/PrimitiveCollection.js index c28e0ed24c2b..f1bb8701b2ba 100644 --- a/packages/engine/Source/Scene/PrimitiveCollection.js +++ b/packages/engine/Source/Scene/PrimitiveCollection.js @@ -189,7 +189,7 @@ PrimitiveCollection.prototype.remove = function (primitive) { delete primitive._external._composites[this._guid]; - if (this.destroyPrimitives) { + if (this.destroyPrimitives && !primitive.isDestroyed()) { primitive.destroy(); } @@ -209,7 +209,7 @@ PrimitiveCollection.prototype.remove = function (primitive) { */ PrimitiveCollection.prototype.removeAndDestroy = function (primitive) { const removed = this.remove(primitive); - if (removed && !this.destroyPrimitives) { + if (removed && !this.destroyPrimitives && !primitive.isDestroyed()) { primitive.destroy(); } return removed; From 8713005edf6bdd7a62e047d1896089792581c37d Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Wed, 13 Dec 2023 20:59:31 -0500 Subject: [PATCH 03/11] Update CHANGES.md with PolylineCollection destroy fixes --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index ba1296873239..b21c8fd7f715 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,7 @@ - Changes the default `RequestScheduler.maximumRequestsPerServer` from 6 to 18. This should improve performance on HTTP/2 servers and above [#11627](https://github.com/CesiumGS/cesium/issues/11627) - Corrected JSDoc and Typescript definitions that marked optional arguments as required in `ImageryProvider` constructor [#11625](https://github.com/CesiumGS/cesium/issues/11625) - The `Quaternion.computeAxis` function created an axis that was `(0,0,0)` for the unit quaternion, and an axis that was `(NaN,NaN,NaN)` for the quaternion `(0,0,0,-1)` (which describes a rotation about 360 degrees). Now, it returns the x-axis `(1,0,0)` in both of these cases. +- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. These changes resolve crashes reported under [#7758] (https://github.com/CesiumGS/cesium/issues/7758) and [#9154] (https://github.com/CesiumGS/cesium/issues/9154) ### 1.112 - 2023-12-01 From 10dd820ed86682d92cefa37ff33d09428da3ac0c Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Wed, 13 Dec 2023 21:00:36 -0500 Subject: [PATCH 04/11] Fix links in CHANGES.md --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index b21c8fd7f715..1f8fc356e147 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,7 +9,7 @@ - Changes the default `RequestScheduler.maximumRequestsPerServer` from 6 to 18. This should improve performance on HTTP/2 servers and above [#11627](https://github.com/CesiumGS/cesium/issues/11627) - Corrected JSDoc and Typescript definitions that marked optional arguments as required in `ImageryProvider` constructor [#11625](https://github.com/CesiumGS/cesium/issues/11625) - The `Quaternion.computeAxis` function created an axis that was `(0,0,0)` for the unit quaternion, and an axis that was `(NaN,NaN,NaN)` for the quaternion `(0,0,0,-1)` (which describes a rotation about 360 degrees). Now, it returns the x-axis `(1,0,0)` in both of these cases. -- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. These changes resolve crashes reported under [#7758] (https://github.com/CesiumGS/cesium/issues/7758) and [#9154] (https://github.com/CesiumGS/cesium/issues/9154) +- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. These changes resolve crashes reported under [#7758](https://github.com/CesiumGS/cesium/issues/7758) and [#9154](https://github.com/CesiumGS/cesium/issues/9154) ### 1.112 - 2023-12-01 From f9ae0c3520fd0bbd9733ffc4861d286a670187ac Mon Sep 17 00:00:00 2001 From: Gabby Getz Date: Fri, 15 Dec 2023 12:20:49 -0500 Subject: [PATCH 05/11] Update CONTRIBUTORS.md --- CONTRIBUTORS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index af7f4d2e4c0f..7f80b77004d7 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -371,3 +371,4 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu - [hongfaqiu](https://github.com/hongfaqiu) - [KOBAYASHI Ittoku](https://github.com/kittoku) - [王康](https://github.com/yieryi) +- [rropp5](https://github.com/rropp5) From 38e4a6e14a96e99a6147bd94c378cc76eef01132 Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Fri, 15 Dec 2023 22:49:26 -0500 Subject: [PATCH 06/11] Distinct PolylineCollections for each CustomDataSource CustomDataSources were using the same PolylineCollection to store their Polyline primitives. When a CustomDataSource was destroyed, it was also destroying the PolylineCollection. Since that object was shared and other CustomDataSources remained in the system, this would cause Cesium to stop rendering when the destroyed PolylineCollection was accessed. Solution: use distinct PolylineCollections per CustomDataSource by keying PolylineCollections with the PrimitiveCollection's guid in addition to the Scene id. --- .../Source/DataSources/PolylineGeometryUpdater.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js index 6c3a595a9aa5..131fdaef3595 100644 --- a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js +++ b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js @@ -661,12 +661,12 @@ function getLine(dynamicGeometryUpdater) { return dynamicGeometryUpdater._line; } - const sceneId = dynamicGeometryUpdater._geometryUpdater._scene.id; - let polylineCollection = polylineCollections[sceneId]; const primitives = dynamicGeometryUpdater._primitives; + const polylineCollectionId = dynamicGeometryUpdater._geometryUpdater._scene.id + primitives._guid; + let polylineCollection = polylineCollections[polylineCollectionId]; if (!defined(polylineCollection) || polylineCollection.isDestroyed()) { polylineCollection = new PolylineCollection(); - polylineCollections[sceneId] = polylineCollection; + polylineCollections[polylineCollectionId] = polylineCollection; primitives.add(polylineCollection); } else if (!primitives.contains(polylineCollection)) { primitives.add(polylineCollection); @@ -869,14 +869,14 @@ DynamicGeometryUpdater.prototype.isDestroyed = function () { DynamicGeometryUpdater.prototype.destroy = function () { const geometryUpdater = this._geometryUpdater; - const sceneId = geometryUpdater._scene.id; - const polylineCollection = polylineCollections[sceneId]; + const polylineCollectionId = geometryUpdater._scene.id + this._primitives._guid; + const polylineCollection = polylineCollections[polylineCollectionId]; if (defined(polylineCollection)) { polylineCollection.remove(this._line); if (polylineCollection.length === 0) { removePolylineCollection(geometryUpdater._scene.primitives, polylineCollection); this._primitives.removeAndDestroy(polylineCollection); - delete polylineCollections[sceneId]; + delete polylineCollections[polylineCollectionId]; } } if (defined(this._groundPolylinePrimitive)) { From d04746f30c5f3917c78bebdc0f0b9f86a7050a8c Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Fri, 15 Dec 2023 22:57:36 -0500 Subject: [PATCH 07/11] Update CHANGES.md --- CHANGES.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 1f8fc356e147..e8d516666182 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,7 +9,8 @@ - Changes the default `RequestScheduler.maximumRequestsPerServer` from 6 to 18. This should improve performance on HTTP/2 servers and above [#11627](https://github.com/CesiumGS/cesium/issues/11627) - Corrected JSDoc and Typescript definitions that marked optional arguments as required in `ImageryProvider` constructor [#11625](https://github.com/CesiumGS/cesium/issues/11625) - The `Quaternion.computeAxis` function created an axis that was `(0,0,0)` for the unit quaternion, and an axis that was `(NaN,NaN,NaN)` for the quaternion `(0,0,0,-1)` (which describes a rotation about 360 degrees). Now, it returns the x-axis `(1,0,0)` in both of these cases. -- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. These changes resolve crashes reported under [#7758](https://github.com/CesiumGS/cesium/issues/7758) and [#9154](https://github.com/CesiumGS/cesium/issues/9154) +- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. +- `PolylineGeometryUpdater` now uses a distinct `PolylineCollection` instance per `CustomDataSource`. This resolves the crashes reported under [#7758](https://github.com/CesiumGS/cesium/issues/7758) and [#9154](https://github.com/CesiumGS/cesium/issues/9154). ### 1.112 - 2023-12-01 From 0f970ee2dce7aa799163f742eda4f8365ea59acb Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Fri, 15 Dec 2023 23:26:23 -0500 Subject: [PATCH 08/11] Revert "Remove destroyed PolylineCollection from Scene #7758" This reverts commit 2189e167818ff75f1c3fd9c9d810ff97c44f6a03. --- .../Source/DataSources/PolylineGeometryUpdater.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js index 131fdaef3595..c0158195ca20 100644 --- a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js +++ b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js @@ -874,7 +874,6 @@ DynamicGeometryUpdater.prototype.destroy = function () { if (defined(polylineCollection)) { polylineCollection.remove(this._line); if (polylineCollection.length === 0) { - removePolylineCollection(geometryUpdater._scene.primitives, polylineCollection); this._primitives.removeAndDestroy(polylineCollection); delete polylineCollections[polylineCollectionId]; } @@ -884,16 +883,4 @@ DynamicGeometryUpdater.prototype.destroy = function () { } destroyObject(this); }; - -function removePolylineCollection(primitiveCollection, toRemove) { - if (defined(primitiveCollection)) { - if (typeof primitiveCollection.remove === "function") { - primitiveCollection.remove(toRemove); - } - - for (let i = 0; i < primitiveCollection.length; i++) { - removePolylineCollection(primitiveCollection.get(i), toRemove); - } - } -}; export default PolylineGeometryUpdater; From 053a7791d23145e290c94dd351f953a57fded772 Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Fri, 15 Dec 2023 23:26:32 -0500 Subject: [PATCH 09/11] Revert "Do not destroy primitives that are already destroyed" This reverts commit f0b36e62ec5814f1ca5df5af97dc5137e557d067. --- .../engine/Source/DataSources/EntityCluster.js | 15 --------------- .../engine/Source/Scene/PrimitiveCollection.js | 4 ++-- 2 files changed, 2 insertions(+), 17 deletions(-) diff --git a/packages/engine/Source/DataSources/EntityCluster.js b/packages/engine/Source/DataSources/EntityCluster.js index baf2156d6ef5..965e19f74a08 100644 --- a/packages/engine/Source/DataSources/EntityCluster.js +++ b/packages/engine/Source/DataSources/EntityCluster.js @@ -921,20 +921,6 @@ EntityCluster.prototype.update = function (frameState) { } }; -/** - * Returns true if this object was destroyed; otherwise, false. - *

- * If this object was destroyed, it should not be used; calling any function other than - * isDestroyed will result in a {@link DeveloperError} exception. - * - * @returns {boolean} True if this object was destroyed; otherwise, false. - * - * @see EntityCluster#destroy - */ -EntityCluster.prototype.isDestroyed = function () { - return false; -}; - /** * Destroys the WebGL resources held by this object. Destroying an object allows for deterministic * release of WebGL resources, instead of relying on the garbage collector to destroy this object. @@ -942,7 +928,6 @@ EntityCluster.prototype.isDestroyed = function () { * Unlike other objects that use WebGL resources, this object can be reused. For example, if a data source is removed * from a data source collection and added to another. *

- * As a result, this object is not destroyed in this function. */ EntityCluster.prototype.destroy = function () { this._labelCollection = diff --git a/packages/engine/Source/Scene/PrimitiveCollection.js b/packages/engine/Source/Scene/PrimitiveCollection.js index f1bb8701b2ba..c28e0ed24c2b 100644 --- a/packages/engine/Source/Scene/PrimitiveCollection.js +++ b/packages/engine/Source/Scene/PrimitiveCollection.js @@ -189,7 +189,7 @@ PrimitiveCollection.prototype.remove = function (primitive) { delete primitive._external._composites[this._guid]; - if (this.destroyPrimitives && !primitive.isDestroyed()) { + if (this.destroyPrimitives) { primitive.destroy(); } @@ -209,7 +209,7 @@ PrimitiveCollection.prototype.remove = function (primitive) { */ PrimitiveCollection.prototype.removeAndDestroy = function (primitive) { const removed = this.remove(primitive); - if (removed && !this.destroyPrimitives && !primitive.isDestroyed()) { + if (removed && !this.destroyPrimitives) { primitive.destroy(); } return removed; From 67c9002b795020797814ee57b22b2dea455751c1 Mon Sep 17 00:00:00 2001 From: rropp5 <12055531+rropp5@users.noreply.github.com> Date: Fri, 15 Dec 2023 23:34:57 -0500 Subject: [PATCH 10/11] Update CHANGES.md --- CHANGES.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index e8d516666182..963051ab0c72 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,8 +9,7 @@ - Changes the default `RequestScheduler.maximumRequestsPerServer` from 6 to 18. This should improve performance on HTTP/2 servers and above [#11627](https://github.com/CesiumGS/cesium/issues/11627) - Corrected JSDoc and Typescript definitions that marked optional arguments as required in `ImageryProvider` constructor [#11625](https://github.com/CesiumGS/cesium/issues/11625) - The `Quaternion.computeAxis` function created an axis that was `(0,0,0)` for the unit quaternion, and an axis that was `(NaN,NaN,NaN)` for the quaternion `(0,0,0,-1)` (which describes a rotation about 360 degrees). Now, it returns the x-axis `(1,0,0)` in both of these cases. -- Updated the `PolylineGeometryUpdater` destroy function to properly remove destroyed `PolylineCollection` instances from the `Scene`. Additionally, updated `PolylineCollection` to address cases where destroy was invoked on a primitive that had already been destroyed. -- `PolylineGeometryUpdater` now uses a distinct `PolylineCollection` instance per `CustomDataSource`. This resolves the crashes reported under [#7758](https://github.com/CesiumGS/cesium/issues/7758) and [#9154](https://github.com/CesiumGS/cesium/issues/9154). +- Fixed an issue where `CustomDataSource` objects all shared a single `PolylineCollection`. Updated `PolylineGeometryUpdater` to use a distinct `PolylineCollection` instance per `CustomDataSource`. This resolves the crashes reported under [#7758](https://github.com/CesiumGS/cesium/issues/7758) and [#9154](https://github.com/CesiumGS/cesium/issues/9154). ### 1.112 - 2023-12-01 From 8b2f4bb91edbdbf65124e04eaa79c1d46de64438 Mon Sep 17 00:00:00 2001 From: Gabby Getz Date: Tue, 16 Jan 2024 09:08:03 -0500 Subject: [PATCH 11/11] run prettier --- .../engine/Source/DataSources/PolylineGeometryUpdater.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js index c0158195ca20..44798c2cc265 100644 --- a/packages/engine/Source/DataSources/PolylineGeometryUpdater.js +++ b/packages/engine/Source/DataSources/PolylineGeometryUpdater.js @@ -662,7 +662,8 @@ function getLine(dynamicGeometryUpdater) { } const primitives = dynamicGeometryUpdater._primitives; - const polylineCollectionId = dynamicGeometryUpdater._geometryUpdater._scene.id + primitives._guid; + const polylineCollectionId = + dynamicGeometryUpdater._geometryUpdater._scene.id + primitives._guid; let polylineCollection = polylineCollections[polylineCollectionId]; if (!defined(polylineCollection) || polylineCollection.isDestroyed()) { polylineCollection = new PolylineCollection(); @@ -869,7 +870,8 @@ DynamicGeometryUpdater.prototype.isDestroyed = function () { DynamicGeometryUpdater.prototype.destroy = function () { const geometryUpdater = this._geometryUpdater; - const polylineCollectionId = geometryUpdater._scene.id + this._primitives._guid; + const polylineCollectionId = + geometryUpdater._scene.id + this._primitives._guid; const polylineCollection = polylineCollections[polylineCollectionId]; if (defined(polylineCollection)) { polylineCollection.remove(this._line);