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

Added bounds property for tileset #345

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JeroenTekle
Copy link

I've added a bounds property to Cesium3DTileset. Personally I use this to teleport to tiles added manually by users.

The bounds calculating code is copied & modified from Cesium3DTileImpl::getBounds. The tileset needs some time to load before this property can be used. _pTileset needs to be set & getRootTile() needs to be available as well. Because of this I now return bounds with all 0's but I don't think this is the most elegant solution. How would you suggest to handle this?

@JeroenTekle
Copy link
Author

@kring is this something of interest for this project or not?

@kring
Copy link
Member

kring commented Jul 19, 2023

Sorry this got by me @JeroenTekle. This seems like a useful feature to me. Based on a quick review though, I think some work needs to be done on the coordinate system. Currently the bounds will be returned as an AABB in ECEF, which is probably not what users need or expect. It should be transformed to the Cesium3DTileset game object's coordinate system, and the documentation of the Bounds property should say as much. You'll notice that FocusTileset does a lot more work around coordinate transformation.

Also, before we can merge this, we do need you to sign the Contributor License Agreement if you haven't already:
https://github.com/CesiumGS/cesium/blob/master/CONTRIBUTING.md#contributor-license-agreement-cla

@JeroenTekle
Copy link
Author

No problem @kring.

For my specific use case I actually need coordinates in ECEF (or GCS) to be able to set the CesiumGeoreference's Origin to the tileset center. I could however convert the game object's local coordinates bounds using CesiumGeoreference::TransformUnityPositionToEarthCenteredEarthFixed.

In this case it might actually be better to expose the root Cesium3DTile through the Cesium3DTileset and use it's bounds property instead? I'm not sure however how to accomplish casting the root tile Cesium3DTilesSelection::Tile* returned by Cesium3DTilesSelection::Tileset::getRootTile() to a DotNet::CesiumForUnity::Cesium3DTile. How would I go about this?

@kring
Copy link
Member

kring commented Jul 24, 2023

In this case it might actually be better to expose the root Cesium3DTile through the Cesium3DTileset and use it's bounds property instead?

That's not unreasonable, but lifetime management will be tricky. You can simply construct a Cesium3DTile instance from the native code, and then set its _pTile property with a pointer to the native Tile instance. See UnityTileExcluderAdaptor.cpp. The trouble is that Tile might later be destroyed, and when it is, trying to use the Cesium3DTile will cause a hard crash. In the case of the tile excluder, we get around this (more or less) by saying in the docs that you're not allowed to hold a Cesium3DTile reference past the end of the ShouldExclude call that provided it. If the root tile is available as a Cesium3DTile property on Cesium3DTileset, there's no obvious valid scope. It's valid until the root tile is destroyed, which will happen on many (but not all) changes to Cesium3DTileset properties.

So I think just providing a way to get the Bounds directly will be simpler for now. But you can definitely implement that by constructing a Cesium3DTile and then getting and returning its bounds.

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.

2 participants