Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix bug in StaticGroundGeometryPerMaterialBatch #7047

Conversation

impactblue573
Copy link
Contributor

@impactblue573 impactblue573 commented Sep 18, 2018

  • Fix a bug in StaticGroundGeometryPerMaterialBatch where invalidated items are removed, causing a local array length to no longer be correct. This leads to calling functions on undefined causing a Cesium render error.

Sandcastle Example of crash

…ms is mutated but loop length is not adjusted accordingly, causing a Cesiumt to crash.
@cesium-concierge
Copy link

Thank you so much for the pull request @impactblue573! 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.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

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

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2018

Thanks for the PR @impactblue573! It looks like this might be a problem in some of the other static batches: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/DataSources/StaticGeometryPerMaterialBatch.js#L376

Can you check all of those and make the same fix if necessary?

Also, do you have a code example to reproduce the crash this fixes?

@impactblue573
Copy link
Contributor Author

@hpinkos Updated those other files, and also included a Sandcastle example of the crash. Thanks!

@hpinkos
Copy link
Contributor

hpinkos commented Sep 21, 2018

👍 thanks @impactblue573, works great!

@hpinkos
Copy link
Contributor

hpinkos commented Sep 21, 2018

I'll update CHANGES.md in master

@hpinkos hpinkos merged commit 1ca5dae into CesiumGS:master Sep 21, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants