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

Add options.id to ModelExperimental #10491

Merged
merged 3 commits into from
Jun 29, 2022
Merged

Add options.id to ModelExperimental #10491

merged 3 commits into from
Jun 29, 2022

Conversation

j9liu
Copy link
Contributor

@j9liu j9liu commented Jun 29, 2022

This PR adds the id option to ModelExperimental, allowing an object to be associated with the model when it is picked. Model stores its pick ids in a _pickIds array, but in ME they are also stored in _pipelineResources (which I renamed from _resources). Is it important to avoid the duplication here?

For testing: this impacts the "Clamp to 3D Tiles" sandcastle, but that uses the Entity API. So to check that it works, go to ModelVisualizer and:

  1. Change Model.fromGltf to ModelExperimental.fromGltf,
  2. Replace the computation in getBoundingSphere: just call BoundingSphere.clone(model.boundingSphere, result), no need to apply the model matrix to it.

@j9liu j9liu requested a review from ptrgags June 29, 2022 15:15
@cesium-concierge
Copy link

Thanks for the pull request @j9liu!

  • ✔️ 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.

@ptrgags
Copy link
Contributor

ptrgags commented Jun 29, 2022

Looks good to me, thanks @j9liu!

@ptrgags ptrgags merged commit 5c80cba into main Jun 29, 2022
@ptrgags ptrgags deleted the model-experimental-id branch June 29, 2022 16:53
@j9liu j9liu mentioned this pull request Jun 30, 2022
51 tasks
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