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

Adds ability to get vctr polyline positions #9684

Merged
merged 4 commits into from
Jul 23, 2021
Merged

Adds ability to get vctr polyline positions #9684

merged 4 commits into from
Jul 23, 2021

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jul 20, 2021

Adds a polylinePositions getter to Cesium3DTileFeature that returns a Float64Array of cartesian coordinates for a polyline in a vctr tile. It works for both clamped and unclamped polylines.

Also adds an option to Cesium3DTileset called vectorKeepDecodedPositions. This option allows positions and offsets to stay in memory even after the positions have been uploaded to the GPU. This must be set to true in order to use this feature. It is false by default so that applications that don't use this feature don't pay the extra memory cost.

Both are marked as @experimental.

There are some limitations:

  • If multiple polylines have the same batch id there isn't a way to distinguish positions in one polyline vs. another. I haven't run across this case in practice. I was considering returning an array of Float64Array but thought it might make the API too complicated.
  • The positions don't update correctly if the tileset model matrix changes. I'm pretty sure this is broken for vector tiles in general because the model matrix is only used once to transform the center point in the beginning (the rotational component is not applied to the positions). I'm not too surprised because the vctr format, like quantized mesh, defines positions in cartographic coordinates so there isn't usually a need to transform the data.
  • Doesn't returned the clamped positions, just the positions as they are in the data

@angrycat9000 @mramato this is good for testing though I still need to finish up the to-do items below:

  • Tests
  • CHANGES.md

Untitled

Sandcastle

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • ❔ 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 force-pushed the polyline-positions branch from c83ad1b to 30e479a Compare July 21, 2021 23:23
@lilleyse lilleyse force-pushed the polyline-positions branch from 30e479a to 209fa30 Compare July 21, 2021 23:29
@lilleyse
Copy link
Contributor Author

Updated with tests. One thing to note is that Vector3DTileContentSpec tests are currently disabled with xdescribe since #8317 so if you're testing locally you'll need to change that to describe. The new tests should pass but some of the existing tests do not, mainly those related to rendering points.

@ebogo1 can you review?

@lilleyse lilleyse requested a review from ebogo1 July 21, 2021 23:35
Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

Tests well in Sandcastle and locally in Jasmine. Are any of the known limitations you listed enough to hold up the PR? Code looks good to me otherwise, I think it's ready after CI passes (just pushed a commit to fix prettier).

@lilleyse
Copy link
Contributor Author

Are any of the known limitations you listed enough to hold up the PR?

No, I think it's fine to proceed without those fixes. There's a little bit of leeway because vector tiles are still marked as experimental in the API.

@ebogo1 ebogo1 merged commit 423ef24 into main Jul 23, 2021
@ebogo1 ebogo1 deleted the polyline-positions branch July 23, 2021 00:31
@junglelan
Copy link

is polygon vctr can use in lastest version? Have any demo data to provide?

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.

4 participants