Fix crashes due to shared PolylineCollection among all CustomDataSources #7758 #9154 #11640
Conversation
|
Thank you so much for the pull request @rropp5! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
|
Hi @rropp5. We'll need a signed Contributor License Agreement (CLA) to review this PR. Please send in a Contributor License Agreement and comment back here to let us know to check this! |
|
@ggetz I just submitted. Thanks! |
|
My second commit addresses a case that I had fixed locally and neglected to push in the original commit. |
|
@ggetz Is there anything else I should do to make this PR as smooth as possible? Thanks! |
|
Apologies for the delay @rropp5! Please sync the latest upstream code and update |
|
@ggetz Apologies for the delay but I just updated the CHANGES.md. Thanks! |
|
@ggetz Oh bummer, this was causing a crash in my system that looked a lot like the problems in those issues. It resolved the crashes I was personally experiencing so I thought it covered those sandcastles as well. I don't have the best environment setup to test sandcastles but I'll have to figure that out moving forward. |
|
@ggetz I figured out how to get the Sandcastle setup locally and did some digging into #7758. I was able to fix that crash by adding a similar fix to the DataSourceDisplay (remove primitives from the scene when they are destroyed). I will try to find some time to test #9154 this weekend and see if I can knock out both issues at once. I'll keep you posted. Thanks! |
|
@ggetz I was able to find the crux of the issue this evening: all CustomDataSources were sharing a single PolylineCollection instance. This created a lot of different cases that would cause the crashes reported under #7758 and #9154. I've updated the PolylineGeometryUpdater to uniquely identify each PolylineCollection for each CustomDataSource and this does fix #7758 and #9154 for me locally. Additionally, after thinking about it, I decided to revert my earlier changes locally to see if they were still necessary and it turns out they aren't. In my opinion, my most recent commit (b7928c9) is the most correct solution so I'm going to revert the earlier commits. Please take a look and let me know if you agree! If you think there's a good reason to keep some of those changes, I'm happy to discuss. Thanks for the help. |
rropp5
changed the title
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.
This reverts commit 2189e16.
This reverts commit f0b36e6.
|
I'm not totally sure, but I think I ran into this problem last night as well, but using CZMLDataSources |
Given the nature of the fix, I think the problem is more specific to the PolylineGeometryUpdater and not so much the CustomDataSource. It just happens that most of the reported issues I found were linked to creating Polylines via the CustomDataSources. |
|
Thanks @rropp5! Sorry for the delay in review here. Happy to see you've identified a fix. Would you mind updating your branch, and moving you entry in |
|
@ggetz done! |
|
Fantastic! Thanks for the fix @rropp5! |
PolylineCollections are destroyed by the PolylineGeometryUpdater but are still available on the Scene and cause Cesium to stop rendering when they are accessed.
This commit ensures the destroyed PolylineCollections are removed from the Scene in the destroy function of PolylineGeometryUpdater so they are not evaluated in the next render cycle of the Scene.
This resolves the crashes reported under #7758 and #9154.