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 polyline update and removal bookkeeping #6455

Merged
merged 9 commits into from
Apr 20, 2018

Conversation

markerikson
Copy link
Contributor

Fixes #4983.

PolylineCollection stores a list of polylines that are waiting to be updated in the next render pass. However, if a polyline is both updated and removed in the same synchronous execution sequence, and the polyline is near the end of the collection's internal this._polylines list, it's possible for the PolylineCollection to effectively read off the end of the array, causing an error "Batch Table instanceIndex out of range".

This is resolved by ensuring that removing a polyline from the collection also removes it from the list of polylines waiting to be updated.

Fixes CesiumGS#4983.

PolylineCollection stores a list of polylines that are waiting to be updated in the next render pass.  However, if a polyline is both updated _and_ removed in the same synchronous execution sequence, _and_ the polyline is near the end of the collection's internal `this._polylines` list, it's possible for the PolylineCollection to effectively read off the end of the array, causing an error "Batch Table instanceIndex out of range".

This is resolved by ensuring that removing a polyline from the collection also removes it from the list of polylines waiting to be updated.
@cesium-concierge
Copy link

Please sign the CLA before we review this PR.

Welcome to the Cesium community @markerikson!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

⚠️ 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.

🌍 🌎 🌏

@markerikson
Copy link
Contributor Author

CLA sent in, and updated CHANGES.md.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 17, 2018

The fix looks good to me! And we got your CLA email. Can you please write a unit test for this?

@markerikson
Copy link
Contributor Author

Yeah, lemme figure out how the unit testing setup works on this project and do so.

@markerikson
Copy link
Contributor Author

Unit test added.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 17, 2018

We received @markerikson's CLA.

@markerikson
Copy link
Contributor Author

Looks like there's some style complaints. I'll fix those.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 17, 2018

@markerikson remember to add yourself to CONTRIBUTORS.md as well =)

@markerikson
Copy link
Contributor Author

And looks like the CI check is clean.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 18, 2018

Great work @markerikson! Thanks so much for digging into this, I wasn't able to figure out what the issue was last time I looked at it.

I just made a few minor styling changes. As soon as CI passes I'll merge this in.

@hpinkos
Copy link
Contributor

hpinkos commented Apr 20, 2018

Great work @markerikson, thanks again! And congrats on your first Cesium contribution!

@hpinkos hpinkos merged commit 1656143 into CesiumGS:master Apr 20, 2018
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.

Batch Table 'instanceIndex is out of range' error when editing a polyline
4 participants