CesiumGS / cesium-unreal

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

Add support for EXT_mesh_gpu_instancing #1371

Closed timoore closed 2 months ago

timoore commented 5 months ago

Create UInstancedStaticMeshComponent objects for meshes that use the EXT_mesh_gpu_instancing extension. Nothing is done yet about metadata.

I used multiple inheritance to share the Cesium data that was in UCesiumGltfPrimitiveComponent with the new UCesiumGltfInstancedComponent class, but this is kind of awkward in the context of Unreal's automatically generated reflection support et al. I'd like to get feedback on whether it would be better to use a composition-based approach instead.

csciguy8 commented 4 months ago

I'd like to get feedback on whether it would be better to use a composition-based approach instead.

While multiple inheritance doesn't seem to be explicitly disallowed in UE's coding standard, from reading discussions on UE forums, it seems that multiply inheriting from interfaces is the general accepted pattern.

I think I understand your approach and seems like it could work. If it was up to my preferences, I'd probably lean toward a single inheritance approach here

timoore commented 3 months ago

I'd like to get feedback on whether it would be better to use a composition-based approach instead.

While multiple inheritance doesn't seem to be explicitly disallowed in UE's coding standard, from reading discussions on UE forums, it seems that multiply inheriting from interfaces is the general accepted pattern.

I think I understand your approach and seems like it could work. If it was up to my preferences, I'd probably lean toward a single inheritance approach here

  • UCesiumGltfPrimitiveComponent derives from UStaticMeshComponent only
  • UCesiumGltfPrimitiveComponent contains an instance of CesiumGltfPrimitiveBase
  • UCesiumGltfInstancedComponent derives from UInstancedStaticMeshComponent only
  • UCesiumGltfInstancedComponent contains an instance of CesiumGltfPrimitiveBase
  • Pros - More UE coding paradigm friendly, no multi-inheritance means no multi-inheritance complexities (problems)
  • Cons - Maybe more duplicated code than you want, but maybe not that bad in the end?

Thanks for the feedback. I'm starting to look at this in detail. The one issue I have with moving to a single inheritance approach -- where UCesiumGltfPrimitiveComponent and UCesiumGltfInstancedComponent don't share a common Cesium base class -- is how to implement UpdateTransformFromCesium(). While it's nice that it's virtual in my current implementation, I suppose a combination of Cast and a template function could get around it. I'm going to continue to play it and try to figure out if multiple inheritance is really that bad here.

kring commented 2 months ago

I noticed a crash while loading an i3dm tileset from the CesiumJS account on Cesium ion, asset ID 3877. Looks like it's dereferencing an empty optional.

image

After a little investigation, I think this is a glTF 1.0 tileset, which cesium-native doesn't support at all, so I don't expect it to work. But it shouldn't crash. It should warn about an unsupported version, like it does for the same situation in b3dm.

kring commented 2 months ago

Your changes look good, thanks @timoore! It looks like you may have missed my comment about the crash above, though. I'll check if that's still an issue and fix it before the release if so. Merging!