CesiumGS / cesium-unity

Bringing the 3D geospatial ecosystem to Unity
https://cesium.com/platform/cesium-for-unity/
Apache License 2.0
347 stars 83 forks source link

Use a component to identify cesium camera #421

Closed raggnic closed 1 week ago

raggnic commented 7 months ago

This PR gives the possibility to distinguish the camera used by cesium with a specific component.

It allows to have one camera to let cesium compute the frustum and the tiles to load. It will not render anything.

The rendering will be done by another camera. It is useful for a platform like the hololens to separate the actual rendering that is always done by the main camera from the tiles retrieval.

All of this is optional and if no camera use the CesiumCamera component Cesium will use the main camera

kring commented 7 months ago

Thanks for the PR @raggnic! Can I please bug you to sign our Contributor License Agreement so that we can review this? https://github.com/CesiumGS/cesium/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla

raggnic commented 6 months ago

I'm waiting for approval of my managers before signing it

Reag commented 4 months ago

@kring , @raggnic Are there still plans to merge this in at some point? Currently, I'm building my own version of Cesium with these changes made, and would love to move to the official releases!

kring commented 4 months ago

@Reag we can't take these changes without a signed CLA, and we recommend that you don't use them, either. @raggnic if the CLA has been signed at this point and I missed it, please let me know.

raggnic commented 4 months ago

Sorry for the delay. I've signed the CLA

kring commented 4 months ago

Thanks @raggnic! We'll review this soon.

kring commented 3 months ago

Thanks for the PR @raggnic, and sorry for the delay in reviewing it! This looks really good for supporting an alternate camera. I think it would be straightforward - and worthwhile! - to extend it to support multiple cameras as well. I think it would just be a matter of making the static field on CesiumCamera a list. And then add the camera attached to the current object to the list in OnEnable, and remove it in OnDisable. What do you think?

In any case, I'm going to move this PR out of tomorrow's release. I definitely want to get it into the August release, though!

raggnic commented 2 months ago

No problem about the delay, I'm not in position to complain :-)

I'm little concerned about having several active cesium cameras, but I'm going to try where it goes

raggnic commented 2 months ago

I've extended the static field to support multiple cameras, but in cameramanager I only consider the first camera of the list I think it will require much more work to have multiple active cameras in cesium, with each defining different frustrums

kring commented 2 months ago

Hi @raggnic, can you elaborate on where you're running into trouble? Multiple cameras with multiple frustums are already supported on the cesium-native side. I think it should just be a matter of calling this line multiple times, once for each camera:

 result.emplace_back(
        unityCameraToViewState(pCoordinateSystem, unityWorldToTileset, camera));
kring commented 2 months ago

Still needs a bit more work, so I'm moving this out of today's release.

raggnic commented 2 months ago

Hi Kevin Sorry for the delay I will have a look as soon as I can

raggnic commented 3 weeks ago

Hi @kring . FYI I've taken your suggestion into account

kring commented 3 weeks ago

Thanks for all your work on this @raggnic!

As I was reviewing this, I realized the collection of cameras should probably be a component rather than a global list, so that we can use different cameras in different scenes. So now there's a CesiumCameraManager that usually lives on the same game object as the CesiumGeoreference.

And then the question became, how do we know which CesiumCameraManager a given CesiumCamera should associate itself with? We can't necessarily require cameras to be nested inside the CesiumGeoreference (this would be inconvenient in some uses-cases). And what if we want to use a single camera with multiple camera managers?

I resolved this by getting rid of the CesiumCamera "marker" component to designate cameras for use with Cesium. Instead, there's an explicit list of cameras on the camera manager: image

The checkboxes allow you to disable the automatic use of the MainCamera and the Editor scene view camera.

As a bonus, I also fixed an unrelated bug where a Cesium3DTileset that is not nested inside a CesiumGeoreference would throw a NullReferenceException when attempting to load.

@raggnic can you take a look and see if you're equally happy with this solution? And now that I've made substantial changes to this PR myself, can I bug @azrogers to give it a review and merge? BTW, the code in this branch is also in the autonomous-cesium-camera branch in the main repo.

raggnic commented 3 weeks ago

For our need in mixed reality the key point is to be able to separate the mainCamera from the one used to select tiles, so I approve this solution. I think that's a much better implementation of this feature, mine was more a quick workaround.

Reag commented 3 weeks ago

Will we be able to edit this component in real time? That is, must it be constant at the unity bootstrap, or can we add or remove cameras depending on user logic? Right now, we are using a variant of Raggnic's code to handle this, as our main server cant actually load cesium. For that server, we have a special asmdef that disables Cesium in its entirety. The clients however have Cesium active, and they inject the appropriate camera after loading a particular scene, to avoid having null references in our servers logs.

kring commented 2 weeks ago

I suggest you try it out with a build from this branch if you have any doubts @Reag. But I think it shouldn't be a problem. The way it works is that the Cesium3DTileset finds the CesiumCameraManager when it's first enabled, but then it asks the camera manager for the set of cameras every frame. So you can programmatically add and remove cameras at will, and the change should take effect on the next frame.

kring commented 2 weeks ago

Thanks for taking a look @raggnic!

kring commented 1 week ago

I've addressed all your comments, so this should be ready for another look. Your nitpicks are always valuable suggestions by the way, not nitpicks at all!

j9liu commented 1 week ago

Thanks @kring ! (and thanks @raggnic for the initial contribution!) Merging now :smile: