Add support for EXT_mesh_gpu_instancing #1371

timoore · 2024-03-07T12:48:48Z

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.

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

csciguy8 · 2024-03-26T18:15:24Z

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?

timoore · 2024-05-02T18:07:01Z

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

Looks great @timoore! I tried it out and it's working well, including when moving the goereference origin. Most of my comments are fairly nitpicky, though I would like to see the multiple inheritance based approach change (sorry!), which will be bigger. Let me know if you have any questions or disagree with anything.

Source/CesiumRuntime/Private/CesiumGltfComponent.cpp

kring · 2024-05-10T05:49:18Z

Source/CesiumRuntime/Private/CesiumGltfComponent.cpp

@@ -2972,14 +3124,17 @@ static void loadPrimitiveGameThreadPart(
    const glm::dmat4x4& cesiumToUnrealTransform,
    const Cesium3DTilesSelection::Tile& tile,
    bool createNavCollision,
-    ACesium3DTileset* pTilesetActor) {
+    ACesium3DTileset* pTilesetActor,
+    std::vector<FTransform>& instanceTransforms) {


Looks like this can be const.

kring · 2024-05-10T06:43:38Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

-class UCesiumGltfPrimitiveComponent : public UStaticMeshComponent {
-  GENERATED_BODY()
-
+class CesiumGltfPrimitiveBase {


Yeah sorry Tim I'm not a huge fan of this multiple inheritance approach. My main complaints are:

It requires some implementation to move to the .h file, and the corresponding headers to be included. I'm pretty big on keeping as much as we can out of the headers because it increases compile time and object file size (we have to ship object files for every platform, so it adds up).

The _uobject bit. It's hacky. Especially since it's public.

Multiple implementation inheritance is widely considered something to avoid.

I mean, these things aren't huge, so if we were getting some significant advantage from it, I'd be willing to let it go. But I don't think we are. The UpdateTransformFromCesium can easily be handled by delegating to a helper function in the two classes. It probably doesn't even need to be a template; I think everything that method uses is available on UPrimitiveComponent which is the common base class.

As for all those public fields in CesiumGltfPrimitiveBase... well, they're a bit of a mess anyway, aren't they? Ideally I'd like to see thoughtful, Blueprint-accessible accessors for the ones that make sense, and the rest made private. But that's a lot of scope creep for this PR, so I'd be happy enough if you just copied them into the UCesiumGltfInstancedComponent (a template function in CesiumGltfCompnent.cpp can be used to set them). Or, better yet, moved them to a common object that is aggregated in both classes (this is a breaking change, but no one is likely to care. We just need to document it). Or... better still... do we even need most of them? A lot of them are probably only relevant for things that aren't supported yet, like metadata. So just leaving them out of the new class for now is an excellent option.

Yeah sorry Tim I'm not a huge fan of this multiple inheritance approach. My main complaints are:

It requires some implementation to move to the .h file, and the corresponding headers to be included. I'm pretty big on keeping as much as we can out of the headers because it increases compile time and object file size (we have to ship object files for every platform, so it adds up).

The _uobject bit. It's hacky. Especially since it's public.

Multiple implementation inheritance is widely considered something to avoid.

While I don't agree with your points in general, I have to agree in the context of an Unreal plugin. _uobject is only needed because regular dynamic_cast isn't available; Unreal turns off rtti. Otherwise, I think multiple inheritance is a fine way to add a common facet of data and behavior to an inheritance chain that has a very obvious "single inheritance" axis. Again, downcasting is very awkward in the Unreal universe.

I've committed the change. I think the the results a messier and more verbose than using multiple inheritance, but then there's no accounting for taste 😄

As for all those public fields in CesiumGltfPrimitiveBase... well, they're a bit of a mess anyway, aren't they?

Not really for me to say.

Ideally I'd like to see thoughtful, Blueprint-accessible accessors for the ones that make sense, and the rest made private. But that's a lot of scope creep for this PR, so I'd be happy enough if you just copied them into the UCesiumGltfInstancedComponent (a template function in CesiumGltfCompnent.cpp can be used to set them). Or, better yet, moved them to a common object that is aggregated in both classes (this is a breaking change, but no one is likely to care. We just need to document it). Or... better still... do we even need most of them? A lot of them are probably only relevant for things that aren't supported yet, like metadata. So just leaving them out of the new class for now is an excellent option.

I'm hoping to use them soon for instances. Maybe we can have another go-around then.

_uobject is only needed because regular dynamic_cast isn't available; Unreal turns off rtti.

I haven't looked at your changes yet, but a quick note here... Unreal has its own RTTI system for UObjects. You can downcast using the Cast<T> function. It works much like dynamic_cast<T>, e.g.:

UObject* pWhatever = ...; UCesiumGltfPrimitiveComponent* pPrimitive = Cast<UCesiumGltfPrimitiveComponent>(pWhatever); if (pPrimitive) { // ... }

Remove the virtual methods of CesiumGltfPrimitiveBase.

The common data between UCesiumGltfPrimitiveComponent and UCesiumGltfInstancedComponent is stored in a _cesiumBase member and accessed using getPrimitiveBase().

…pu_instancing

In the tests, particularly in CesiumMetadataPickingBlueprintLibrary.cpp, the pBase variable for common Cesium data was only initialized in the first BeforeEach form.

kring

Thanks for getting rid of the multiple inheritance. A few comments on the new approach, though...

kring · 2024-05-21T08:44:02Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

-  // Sets default values for this component's properties
-  UCesiumGltfPrimitiveComponent();
-  virtual ~UCesiumGltfPrimitiveComponent();
+  CesiumGltfPrimitiveBase();


This needs a better name, because Base is misleading now that it's no longer a base class. My best suggestion is CesiumTileData. Eventually this will probably become a Blueprint-accessible USTRUCT, but that doesn't need to be part of this PR.

I finally chose CesiumPrimitiveData. CesiumTileData doesn't seem write as the data for each primitive, not the whole tile.

Oh yes, good point. CesiumPrimitiveData works for me.

kring · 2024-05-21T09:33:37Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

-   * @param CesiumToUnrealTransform The new transformation.
-   */
-  void UpdateTransformFromCesium(const glm::dmat4& CesiumToUnrealTransform);
+  void BeginDestroyPrimitive();


This could use a better name. The overridden method on the actual UObjects has to be called BeginDestroy to hook into Unreal, but here the Begin part doesn't make much sense. Something like Destroy or Clear would be fine IMO.

kring · 2024-05-21T09:42:41Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

 };
+
+CesiumGltfPrimitiveBase* getPrimitiveBase(USceneComponent* pComponent);


This should be a virtual method on an interface (perhaps ICesiumTile) that is implemented by both primitive types, rather than a global function that uses a dynamic cast. See here for how to create interfaces in UE:
https://dev.epicgames.com/documentation/en-us/unreal-engine/interfaces-in-unreal-engine?application_version=5.2

Thinks for that link, it's very helpful. I could note that its example shows inheriting from the interface via multiple inheritance, but that would be unproductive 😎

I could note that its example shows inheriting from the interface via multiple inheritance

Sure, that's the only way to implement interfaces in C++. Multiple inheritance is widely considered acceptable when it's inheritance of an interface only (no implementation, no fields). Inheriting from multiple classes with fields has extra gotchas.

This goes back to Effective C++, item 40 if my quick google search is to be believed (I don't have the book handy).

As always with these sorts of rules of thumb ("don't use multiple inheritance, unless all but one is an interface"), there are exceptions. But this isn't one of them.

Setting snark aside, I think the most idiomatic approach in Unreal would be to define an interface class, that holds the primitive data, and use it via multiple inheritance. However, we would access the data members through a virtual function and not with direct access.

kring · 2024-05-21T09:45:38Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

+};
+
+template <typename UnrealType>
+void UpdateTransformFromCesium(


This function and BeginDestroyPrimitive below should be moved to the .cpp file and the anonymous namespace. If they need to be called externally, they should be added to the ICesiumTile interface (see above), and the virtual methods can be implemented by calling these templatized functions.

It looks like this hasn't been addressed. I still think this function should be moved to the new ICesiumPrimitive interface. Then you can remove the very implementation-detaily resetPhysicsTransform from that interface. In the implementation file, implement UpdateTransformFromCesium by delegating the common bits to a common function in the anonymous namespace.

kring · 2024-05-21T09:46:41Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

+}
+
+template <typename T>
+class CesiumGltfPrimitive : public CesiumGltfPrimitiveBase {


Is this still used? Doesn't look like it.

Completing the circle, this commit introduces an interface class ICesiumPrimitive, defined using Unreal's UINTERFACE macro and auto-generated headers. It's used via multiple inheritance in the the Cesium primitive component classes. The CesiumPrimitiveData class is stored in and accessed through it.

kring · 2024-05-30T01:58:52Z

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.

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

A few last details, but this is looking really close! If you're able to address these comments Thursday or Friday, we should be able to include it in the release on Monday.

Source/CesiumRuntime/Private/CesiumPrimitive.h

kring · 2024-05-30T03:54:03Z

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

+};
+
+template <typename UnrealType>
+void UpdateTransformFromCesium(


It looks like this hasn't been addressed. I still think this function should be moved to the new ICesiumPrimitive interface. Then you can remove the very implementation-detaily resetPhysicsTransform from that interface. In the implementation file, implement UpdateTransformFromCesium by delegating the common bits to a common function in the anonymous namespace.

Source/CesiumRuntime/Private/CesiumGltfComponent.cpp

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.cpp

Source/CesiumRuntime/Private/CesiumGltfPrimitiveComponent.h

The function is now implemented by the Cesium mesh components with the aid of a helper template function.

uobject was a pointer to some kind of Unreal Component type, so the name was changed to pComponent.

…pu_instancing

timoore added 2 commits March 7, 2024 13:35

Add support for EXT_mesh_gpu_instancing

9093bbd

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

clang-format changes

5712a39

timoore added 2 commits April 30, 2024 13:26

Merge remote-tracking branch 'origin/main' into mesh_gpu_instancing

8785b9d

Merge remote-tracking branch 'origin/main' into mesh_gpu_instancing

580b191

kring added 2 commits May 10, 2024 09:40

Merge remote-tracking branch 'origin/main' into mesh_gpu_instancing

77762db

Use the i3dm-2024 branch of cesium-native.

ae6f37b

kring self-requested a review May 10, 2024 04:12

Formatting.

81a7926

kring requested changes May 10, 2024

View reviewed changes

timoore added 6 commits May 13, 2024 16:38

First steps towards changing multiple inheritance to composition

d1d3827

Remove the virtual methods of CesiumGltfPrimitiveBase.

Change multiple inheritance of Cesium data to composition

08a5e0d

The common data between UCesiumGltfPrimitiveComponent and UCesiumGltfInstancedComponent is stored in a _cesiumBase member and accessed using getPrimitiveBase().

Respond to code review feedback

718d113

Added note to CHANGES.md

8047770

Merge remote-tracking branch 'origin/mesh_gpu_instancing' into mesh_g…

1b74e50

…pu_instancing

Merge straggler

a5a3b44

timoore marked this pull request as ready for review May 14, 2024 11:50

timoore added 3 commits May 14, 2024 14:02

Remove virtual destructor from CesiumGltfPrimitiveBase

61f60a8

Run format

a97f2ca

Initialize pBase properly in tests

2d4d288

In the tests, particularly in CesiumMetadataPickingBlueprintLibrary.cpp, the pBase variable for common Cesium data was only initialized in the first BeforeEach form.

kring requested changes May 21, 2024

View reviewed changes

timoore added 4 commits May 21, 2024 14:43

Rename CesiumGltfPrimitiveBase to CesiumPrimitiveData

fbf3b6b

Update cesium-native

e3ab686

Formatting changes

6a7e37b

timoore requested a review from kring May 28, 2024 16:23

Merge remote-tracking branch 'origin/main' into mesh_gpu_instancing

78304aa

kring added this to the June 2024 Release milestone May 30, 2024

kring reviewed May 30, 2024

View reviewed changes

lilleyse mentioned this pull request May 30, 2024

Set bUsedWithInstancedStaticMeshes in the default Cesium material CesiumGS/cesium-unreal-samples#59

Open

timoore added 8 commits May 30, 2024 22:02

Change getPrimitiveData() to return a reference

6d77b8b

Move UpdateTransformFromCesium() into ICesiumPrimitive

6ca4717

The function is now implemented by the Cesium mesh components with the aid of a helper template function.

Change uobject parameter name to conform to coding standard

8482947

uobject was a pointer to some kind of Unreal Component type, so the name was changed to pComponent.

Cleanup

b2fd967

Move _cesiumData out of interface class to Cesium component classes

b52b630

rewrite primitive CalcBounds() to use helper function

640c74b

Fix formatting

ebe1ccf

Merge remote-tracking branch 'origin/mesh_gpu_instancing' into mesh_g…

5f41528

…pu_instancing

timoore requested a review from kring May 31, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for EXT_mesh_gpu_instancing #1371

Add support for EXT_mesh_gpu_instancing #1371

timoore commented Mar 7, 2024

csciguy8 commented Mar 26, 2024 •

edited

timoore commented May 2, 2024

kring left a comment

kring May 10, 2024

kring May 10, 2024

timoore May 14, 2024

kring May 15, 2024

kring left a comment

kring May 21, 2024

timoore May 21, 2024

kring May 21, 2024

kring May 21, 2024

kring May 21, 2024

timoore May 21, 2024 •

edited

kring May 21, 2024

timoore May 21, 2024

kring May 21, 2024

kring May 30, 2024

kring May 21, 2024

kring commented May 30, 2024

kring left a comment

kring May 30, 2024

		};

		CesiumGltfPrimitiveBase* getPrimitiveBase(USceneComponent* pComponent);

Add support for EXT_mesh_gpu_instancing #1371

Are you sure you want to change the base?

Add support for EXT_mesh_gpu_instancing #1371

Conversation

timoore commented Mar 7, 2024

csciguy8 commented Mar 26, 2024 • edited

timoore commented May 2, 2024

kring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoore May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kring commented May 30, 2024

kring left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csciguy8 commented Mar 26, 2024 •

edited

timoore May 21, 2024 •

edited