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

Make root tile public #6944

Merged
merged 4 commits into from
Aug 23, 2018
Merged

Make root tile public #6944

merged 4 commits into from
Aug 23, 2018

Conversation

lilleyse
Copy link
Contributor

Makes tileset.root public as a convenience. Cesium3DTile is already part of the public API so this is safe. A lot of code outside of Cesium is calling tileset._root and it would be better to call tileset.root instead. I don't see the harm in making it public.

Also added tile.boundingVolume since we already have tile.contentBoundingVolume. They are both @private still.

@OmarShehata can you review?

@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

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

🌍 🌎 🌏

@OmarShehata
Copy link
Contributor

Looks good, Cesium Viewer still works in all browsers and in 2D/CV.

Since this does expose a public way to access root, should it be added to CHANGES.md ?

@lilleyse
Copy link
Contributor Author

Ah yes, added an entry to CHANGES.md.

*
* @type {TileBoundingVolume}
* @readonly
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

Why bother adding this if it's marked @private ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty much just following what contentBoundingVolume does. It looks nicer when other files that need this call tile.boundingVolume instead of tile._boundingVolume. It can't be public though since TileBoundingVolume is private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay. We want to access it from other classes without adding it to the public API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup

@hpinkos
Copy link
Contributor

hpinkos commented Aug 23, 2018

Thanks @lilleyse!

@hpinkos hpinkos merged commit 9287c3c into master Aug 23, 2018
@hpinkos hpinkos deleted the root-public branch August 23, 2018 21:37
@hpinkos
Copy link
Contributor

hpinkos commented Aug 23, 2018

Thanks for reviewing @OmarShehata!

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