search

CesiumGS / cesium-unreal

Bringing the 3D geospatial ecosystem to Unreal Engine
https://cesium.com/platform/cesium-for-unreal/
Apache License 2.0
921 stars 295 forks source link

Add support for custom ellipsoids #1432

Closed azrogers closed 3 months ago

azrogers commented 4 months ago

Currently, Cesium for Unreal is hardcoded to use the WGS84 ellipsoid. This change adds support for specifying other ellipsoids by creating assets for them and assigning them to georeferences.

Closes #366

csciguy8 commented 3 months ago

One main issue that needs to be fixed before this can be merged in is that changing the ellipsoid of a georeference should cause all of its associated tilesets to be reloaded.

@azrogers , just double checking, is this still an issue?

(you've marked this PR as ready, yet I saw this comment in the description)

csciguy8 commented 3 months ago

Found a crash, I think it's valid?

Was just playing around with the Unreal Editor Details window and ellipsoid options. Debug build, UE 5.4.1

image

csciguy8 commented 3 months ago

Found another crash, but in UE 5.2 (still debug build, also happens in 5.4).

Opened up the 01 sample level, then in the Outliner window, find "Edit DynamicPawn", then click it.

Obviously not directly related to ellipsoids, but triggers a related UI refresh I'm guessing...

image

azrogers commented 3 months ago

@csciguy8 Addressed your review comments. Fixed crashes related to the UCesiumGlobeAnchorComponent details editor and added a bunch more checks so that you should get errors logged instead of crashes.

csciguy8 commented 3 months ago

Thanks @azrogers for the changes.

Two comments remain that got lost in the shuffle...

1) Is the issue mentioned in the description still an issue that needs to be addressed? (the ellipsoid of a georeference should cause all of its associated tilesets to be reloaded)

2) GetNativeEllipsoid is still returning a TSharedPtr even though we don't use shared pointer functionality.

azrogers commented 3 months ago

In response to the point in my original post, that should be fixed. The tileset should be reloaded when the ellipsoid is changed.

I've removed the usage of TSharedPtr. Let me know if there's any final things you need me to take care of @csciguy8

azrogers commented 3 months ago

Oh, I should also mention at some point that this PR will close #366.

csciguy8 commented 3 months ago

Did some testing and good news, no crashes!

Looks pretty much done, but I did find 2 issues with the tilesets refreshing...

1) When changing which ellipsoid is used through the "Details" window, I noticed that the globe shape updated, but the tiles didn't look to reload. If I hit the "Refresh Button" on the tileset manually, things do what I expect. I even set a breakpoint in ACesium3DTileset::HandleOnGeoreferenceEllipsoidChanged and noticed it wasn't getting trigged.

2) Similarly, if I make a new ellipsoid, let the georeference use it, then edit it in its new Details window, then save, I'm not seeing the tileset refresh either.

azrogers commented 3 months ago

@csciguy8 Made GetNativeEllipsoid return a const ref, made UCesiumEllipsoid store a TOptional instead of a TUniquePtr, fixed the OnEllipsoidChanged event (apparently the handler needs to be marked with "CallInEditor"), and added a method that reloads any georeferences using an ellipsoid asset when that ellipsoid asset is saved. Should be good to go!