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

Fix terrain clipping planes sandcastle example #6367

Merged
merged 1 commit into from
Mar 24, 2018
Merged

Fix terrain clipping planes sandcastle example #6367

merged 1 commit into from
Mar 24, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Mar 23, 2018

Fixes #6366

Users should never scene.primitives.removeAll when they're using the entity layer.

I'm worried this might be a common problem though. The reason this crash was introduced is because DataSources have their own PrimitiveCollection now. Calling scene.primitives.removeAll removes and destroys the data source's primitive collection from primitives. Then later on StaticOutlineGeometryBatch tried to remove a primitive from the destroyed PrimitiveCollection on the DataSource and that's where the error came from.

I don't know a way around this. Do you think we should add a private PrimitiveCollection on scene that all the entitiy primitives get dumped into? That way the end users wouldn't accidentally delete them out from underneath the entity layer.

@cesium-concierge
Copy link

Signed CLA is on file.

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Mar 23, 2018

This has been a known problem forever, so don't stress too much. Basically, no one should ever call scene.primitives.removeAll.

@hpinkos
Copy link
Contributor Author

hpinkos commented Mar 23, 2018

Okie dokie. I was just concerned because before users would just be like 'why can't I see my polygon' and now they'll be like 'why is my whole application crashing?'

@mramato
Copy link
Contributor

mramato commented Mar 23, 2018

There were plenty of other scenarios where this would lead to a crash (for example, calling removeAll while entities were still in the scene crashes in most cases).

@mramato mramato merged commit b9e8138 into master Mar 24, 2018
@mramato mramato deleted the fix-6366 branch March 24, 2018 03:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants