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.

Sign up for GitHub

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

Jump to bottom

8321/polyline crash #8546

Merged
merged 14 commits into from
Jan 23, 2020
Merged

8321/polyline crash #8546

merged 14 commits into from
Jan 23, 2020

Conversation

Samulus
Copy link
Contributor

@Samulus Samulus commented Jan 16, 2020

Fixes #8321

This crash is caused by polyLinesToUpdate becoming out of sync with polyLines. If the position of a polyLine is marked as dirty (i.e .position = .... is invoked) and the polyline is marked for removal, the polyLine will remain in the polyLinesToUpdate array, but not the main polyLines array. The arrays become out of sync and a failure occurs when we try to update a deleted polyLine

Tested on Linux / Nvidia

Test case here: http://localhost:8080/Apps/Sandcastle/index.html#c=lZJPb5wwEMW/yohLWGllkuyty64akUukSKnUP5eSgwOzWyvGRmPDilb73TtgWEKiHsrJ9rx583s2rSRoFZ6QYAcGT5ChU00lfgxn8VUxbDNrvFQG6Wq1zU1uWu4qrNZYeGXNsvOL1Z1mbXapx31TGCJcgQZFTapSXrXohCzLeLYK9kkCGaH0CPVo5oDw2GhJusuNQ/9gPFIrdVwMumnmGm6ug8WhMYFtKYhX8Cc3wF8fYXLnADPCQDSK+q+2TvXn7tOUMJPkeSXNRhzIVvd4JER3RyS7+Oc1M6zhdg2b51UwOQeifsnBvtdlCDa6gj14NDNTM9SnfEz2Nu0l1ZwjIIYcYnbd/TfsdqSd73AkvmNAAgleVXzBzts6QCpzBGlKfpnKthjkDPuNZbbx/2ItNEq6BFqmnRgG3fwgYUA8pVyQDqjn3ETrKHW+07gPxc+qqi15aEjHQiQeq1rzJJe8NMUrelE413emydSUlqoFVe7y6N0vn0fMLJ3jyqHR+qv6jXm0TxPWL9q0lSVfyVOLpGXXS37d7B/DoRAiTXj7sctbq18kvXH8Cw

@Samulus Samulus requested a review from lilleyse January 16, 2020 15:22
@cesium-concierge
Copy link

Thanks for the pull request @Samulus!

  • ✔️ 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.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

lilleyse
lilleyse reviewed Jan 16, 2020
View reviewed changes
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix! Mostly small style comments and one larger organizational idea from me.

Source/Scene/PolylineCollection.js Outdated Show resolved Hide resolved
Source/Scene/PolylineCollection.js Outdated Show resolved Hide resolved
Source/Scene/PolylineCollection.js Outdated Show resolved Hide resolved
Source/Scene/PolylineCollection.js Show resolved Hide resolved
Source/Scene/PolylineCollection.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

Also, is it possible to find a reproducible test case that could be unit tested?

@mramato
Copy link
Contributor

mramato commented Jan 16, 2020

Awesome work @Samulus.

@tinco can you confirm that this branch fixes your issue?

@tinco
Copy link
Contributor

tinco commented Jan 16, 2020

Epic, I'll try swapping over our production to this release, hopefully there's nothing incompatible going on in master besides this fix :) If it's fixed I should see no more occurrences from today on, I'll know by wednesday next week for sure.

@tinco
Copy link
Contributor

tinco commented Jan 16, 2020

Hm I can't get master to work, something changed with esm, I get errors about not being able to resolve fs and module, which makes sense since we're trying to run this in the browser, but I'm not sure how to fix this. Is this no longer how you use webpack with cesium?: https://gist.github.com/tinco/8f423938bb9dd4fc8edc1fa86ab017b3

@mramato
Copy link
Contributor

mramato commented Jan 16, 2020

@tinco not sure about your setup, but see https://github.com/AnalyticalGraphicsInc/cesium-webpack-example/blob/master/webpack.config.js for a known working configuration with ES6 versions of Cesium. (That whole repository is a standalone example with both debug and release configs, I linked to the debug one I think)

@Samulus Samulus force-pushed the 8321/polyline_crash branch from 64eee34 to 17236a3 Compare January 16, 2020 18:45
@tinco
Copy link
Contributor

tinco commented Jan 16, 2020

Just tested master on that repo @mramato, it fails there as well, so I guess something really is broken on master for webpack!

@Samulus Samulus force-pushed the 8321/polyline_crash branch from 17236a3 to 8c9f01e Compare January 16, 2020 20:26
@mramato
Copy link
Contributor

mramato commented Jan 16, 2020

@tinco I was unable to reproduce. To avoid derailing this issue, can you write up an issue at https://github.com/AnalyticalGraphicsInc/cesium-webpack-example/issues with details. Thanks!

@Samulus
Copy link
Contributor Author

Samulus commented Jan 16, 2020 •
edited
Loading

Also, is it possible to find a reproducible test case that could be unit tested?

@lilleyse I think that the PolylineCollectionSpec.prototype.update function combined with setInterval is critical to replicating this error. I tried a bunch of different permutations of mutating the collection without calling .update inbetween and couldn't reproduce the same error we see in the Sandcastle demo. Is there a (sane) way of mocking out a frameState in a Jasmine test?

lilleyse
lilleyse reviewed Jan 17, 2020
View reviewed changes
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bunch of small suggestions. The new approach looks really clean.

Source/Scene/Polyline.js Outdated
* Gets the destruction status of this polyline
* @memberof Polyline.prototype
* @type {Boolean}
* @default undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @default undefined
* @default false

Specs/Scene/PolylineCollectionSpec.js Outdated
polylines.remove(secondPolyline);
polylines.update(scene._frameState);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
polylines.update(scene._frameState);
polylines.update(scene.frameState);

Specs/Scene/PolylineCollectionSpec.js Outdated

expect(polylines._polylinesToUpdate.length).toEqual(2);

polylines.remove(secondPolyline);
polylines.update(scene._frameState);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
polylines.update(scene._frameState);
polylines.update(scene.frameState);

Source/Scene/PolylineCollection.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

Oh, and please update CHANGES.md!

@Samulus
Copy link
Contributor Author

Samulus commented Jan 21, 2020

@lilleyse All comments addressed 👍

@Samulus Samulus force-pushed the 8321/polyline_crash branch from 7f4a3a1 to 080a3fa Compare January 21, 2020 14:27
@lilleyse
Copy link
Contributor

Looks great, thanks @Samulus!

@lilleyse lilleyse merged commit 3f35764 into master Jan 23, 2020
@lilleyse lilleyse deleted the 8321/polyline_crash branch January 23, 2020 16:10
@tinco tinco mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilleyse lilleyse lilleyse left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PolylineCollection update instanceIndex is out of range
5 participants
@Samulus @cesium-concierge @lilleyse @mramato @tinco