[enhancement#1658] mesh, material and tile loading callbacks #1669

GhisBntly · 2025-05-09T07:50:06Z

Proposed implementation for #1658

j9liu

Thanks for opening this for review @GhisBntly ! Just looking at this from a high level, I had a few questions about some of the choices made. But I think this approach is promising 🤞

Source/CesiumRuntime/Public/Cesium3DTileset.h

j9liu · 2025-05-09T18:52:15Z

Source/CesiumRuntime/Public/CesiumLoadedTile.h

+class UStaticMeshComponent;
+
+UINTERFACE()
+class UCesiumLoadedTileBase : public UInterface {


I prefer this interface over exposing CesiumGltfPrimitiveComponent, so thanks for working on it! I might add more feedback later, but I think this approach works for now.

j9liu · 2025-05-09T19:01:52Z

Source/CesiumRuntime/Private/CesiumGltfComponent.h

  UPROPERTY(EditAnywhere, Category = "Rendering")
  FCustomDepthParameters CustomDepthParameters{};

+  Cesium3DTilesSelection::Tile* pTile = nullptr;


I know why you did this, but I'm hesitant to give this component the power to modify the Native tile. I'm not sure if this could affect the tileset loading algorithm in some unexpected way... maybe @kring would know off the top of his head?

Can you explain when you would use SetRenderReady for your use case? I'm hoping there's a better or equal way to achieve this without reaching into the Native tile state 🤔

I think I'm generally ok with the Tile being accessible, so long as it's only accessible from the main thread. The public API of Tile could definitely be better, to make it clear what sort of modifications are ok, but as an advanced API here I think it's fine for now.

I would like to understand better why the "render ready" concept is used, and why it's needed, though.

Yeah adding this member made me uncomfortable too ;-) But I didn't see any other way, and at least UCesiumGltfComponent is Private. The Public interface I added only gives access to the Tile ID and to the setter I needed (SetRenderReady).

Now for the use case: it will be complex to explain, as we reached this solution after much trial and error...
Our app logic means we need to create textures for some of the mesh components. Initially, we were doing everything in OnMeshConstructed:

discover per-vertex metadata to identify mesh subsets and map them to app domain data

in tiles where it was needed, create property textures (with a size matching the max FeatureID found on vertices), initialized (UTexture2D::UpdateTextureRegions) with app-specific data used by our custom material's shaders

assign (SetTextureParameterValueByInfo) these textures to all materials of the tile's meshes

But we realized that, randomly, some textures were not coming through properly, ie. the render was showing as if the material was accessing garbage data, even though we had triple-checked that all data was correct and the right methods called on our side. We finally found that we had to wait for the render thread to process the render command enqueued by the UpdateTextureRegions call to guarantee that the texture could be assigned to the materials :-/

So we added a synchronization mechanism through UpdateTextureRegions's DataCleanupFunc parameter: to avoid blocking the game thread while waiting for the RHI thread, we had to defer the finalization of our materials setup to the next tick which in turn meant we had to tell the tiles selection algorithm that the tile was not quite ready for render yet.

Thanks for the explanation @GhisBntly! I'm still a little confused as to why it's necessary to delay, though.

UpdateTextureRegions enqueues a command to the renderer thread, that makes sense. But as long as any draw commands for that tile are executed by the render thread after that command enqueued by UpdateTextureRegions, I don't think any particular synchronization or delay should be necessary. Certainly this is true when we create textures, at least.

Is it possible something else was happening here? Like the UpdateTextureRegions wasn't invoked until after the tile was drawn with the non-updated texture somehow?

Or is there more going on in UpdateTextureRegions than I expect?

I agree our problem and solution doesn't feel "right" ... but we have spent a fair amount of time debugging and didn't find anything so far that could be a bug in our code. Our assumption is that assigning the texture with SetTextureParameterValueByInfo has immediate side effects on render commands already enqueued :-/

It was our early days with Unreal last year so maybe we missed something. If you think this is too intrusive a change to merge without a better understanding of what's happening, I can remove that part from the PR, and we'll open a new one with it later if need be, after we have had another look at it with the experience gained.

kring

Thanks for opening this PR @GhisBntly! I have a bunch of small comments, but overall it looks great.

Source/CesiumRuntime/Private/CesiumGltfComponent.cpp

Source/CesiumRuntime/Public/CesiumMeshBuildCallbacks.h

Source/CesiumRuntime/Public/CesiumLoadedTile.h

Source/CesiumRuntime/Public/Cesium3DTileset.h

Source/CesiumRuntime/Private/CesiumGltfComponent.cpp

kring · 2025-05-12T04:56:26Z

Source/CesiumRuntime/Private/CesiumGltfComponent.h

  UPROPERTY(EditAnywhere, Category = "Rendering")
  FCustomDepthParameters CustomDepthParameters{};

+  Cesium3DTilesSelection::Tile* pTile = nullptr;


I think I'm generally ok with the Tile being accessible, so long as it's only accessible from the main thread. The public API of Tile could definitely be better, to make it clear what sort of modifications are ok, but as an advanced API here I think it's fine for now.

I would like to understand better why the "render ready" concept is used, and why it's needed, though.

Left to do: * get rid of bNeedsCPUAccess * replace TileID by ... ? * decide about CustomizeGltfMaterial * adapt UCesium3DTilesetLifecycleEventReceiver's signatures to be implementable from a BP(??)

…allbacks

… sense of glTF vertices

GhisBntly · 2025-06-11T14:26:49Z

Note: I had to add GetVertexPositionScaleFactor() to the UCesiumLoadedTile interface in order to get rid of CPU access to Unreal memory buffers in our implementation of the lifecycle events receiver: the function replicates the logic hardcoded in loadPrimitive (multiplication by the internal CesiumPrimitiveData::positionScaleFactor as well as Y axis inversion).
I could have added a vertex visiting method instead in UCesiumLoadedTilePrimitive, but it didn't really seem worth the trouble(?)

GhisBntly · 2025-06-11T14:30:59Z

I mark this PR as ready for review, although I'm still unsure about SetRenderReady. I will try to confirm (or debunk ;-)) our understanding of the problem with a simpler example tomorrow. In particular, I'm indeed keen on seeing whether the problem could also happen with cesium-created textures :-/

GhisBntly · 2025-06-12T16:08:57Z

About SetRenderReady, note that I also considered using it to avoid blocking the game thread for too long, because each tile needs substantial post-processing to be made "render-ready" (parsing vertex metadata, map this data to internal domain data, create optimization structures for 4D schedule replay, and finally create the textures for the custom material shader...).

The method that does all that in our code already supports a time budget: even though I do not use it for this purpose at the moment, I may want to do so in the future to smooth out tile transitions, which may be a valid reason to include this feature?

* give access to Tileset actor

GhisBntly · 2025-06-17T12:09:18Z

Sadly, I was unable to reproduce the specific issue that led us to defer material texture assignments, but it doesn't mean the problem no longer happens, as it was witnessed only in rare circumstances, and the project data I had used to investigate it no longer exists :-(
Still, I would really prefer keeping SetRenderReady, if only to allow for better distribution of the tile post-process load on our end over several game ticks.

j9liu · 2025-07-08T14:36:03Z

Sorry for the delay on these PRs @GhisBntly :( I'll take a look at this sometime this week!

kring · 2025-07-22T08:07:29Z

The method that does all that in our code already supports a time budget: even though I do not use it for this purpose at the moment, I may want to do so in the future to smooth out tile transitions, which may be a valid reason to include this feature?

cesium-native already has (and cesium-unreal already uses) a main thread loading time limit:
https://github.com/CesiumGS/cesium-native/blob/v0.49.0/Cesium3DTilesSelection/include/Cesium3DTilesSelection/TilesetOptions.h#L302

Also, if you can do any of the work in a worker thread, then that process is allowed to take place over multiple frames, with a Future signaling when it's complete.

Does the "render ready" mechanism satisfy a use-case that those two mechanisms don't already satisfy?

Sadly, I was unable to reproduce the specific issue that led us to defer material texture assignments, but it doesn't mean the problem no longer happens, as it was witnessed only in rare circumstances, and the project data I had used to investigate it no longer exists :-(

It makes me a little uneasy, because the use-case is unclear (at least to me) and its the sort of thing that has a moderately high potential for error by causing violation of assumptions in the tile selection algorithm. From what I can see, this PR doesn't actually use the "render ready" flag at all. So it should be pretty easy to extract that part into a separate PR, right?

kring

Lots of small comments here, but once again, it looks great overall!

CHANGES.md

Source/CesiumRuntime/Public/Cesium3DTileset.h

Source/CesiumRuntime/Public/Cesium3DTilesetLifecycleEventReceiver.h

Source/CesiumRuntime/Public/CesiumLoadedTile.h

Source/CesiumRuntime/Public/CesiumMetadataValue.h

GhisBntly · 2025-07-22T13:09:22Z

Also, if you can do any of the work in a worker thread, then that process is allowed to take place over multiple frames, with a Future signaling when it's complete.

Does the "render ready" mechanism satisfy a use-case that those two mechanisms don't already satisfy?

No you're right, my time budget use case doesn't add anything indeed.

It makes me a little uneasy, because the use-case is unclear (at least to me) and its the sort of thing that has a moderately high potential for error by causing violation of assumptions in the tile selection algorithm. From what I can see, this PR doesn't actually use the "render ready" flag at all. So it should be pretty easy to extract that part into a separate PR, right?

I understand, I will remove it from this PR. I have started testing again our app without this deferred texture setup system, I guess if it proves stable and no longer show any bug like the one we had, we may remove it entirely on our side too in the future.

…allbacks

Which was not possible with a TObjectPtr<ICesium3DTilesetLifecycleEventReceiver>: tried a TScriptInterface, but decided for a UObject after reading about caveats: see comments over UCesium3DTilesetLifecycleEventReceiver. Also flagged UCesium3DTilesetLifecycleEventReceiver with 'CannotImplementInterfaceInBlueprint'

This reverts commit 380fa13. I was supposed to remove the F from our version, not add it to the upstream... ;^^

and from SetGltfParameterValues, which it probably mimicked in the first place

…fAccessor

GhisBntly · 2025-08-14T15:15:59Z

@kring , I believe I have accounted for all comments so far - ready for next round ;^^

[enhancement#1658] mesh, material and tile loading callbacks

8b92701

CesiumGS#1658

GhisBntly marked this pull request as draft May 9, 2025 08:06

j9liu mentioned this pull request May 9, 2025

Extension points for mesh- and material-related customizations #1658

Open

4 tasks

j9liu reviewed May 9, 2025

View reviewed changes

kring reviewed May 12, 2025

View reviewed changes

GhisBntly mentioned this pull request May 13, 2025

Add a "render-readiness" flag on tiles CesiumGS/cesium-native#1177

Open

GhisBntly added 8 commits May 13, 2025 21:22

Account for most of the feedback received to this day

6025fa9

Left to do: * get rid of bNeedsCPUAccess * replace TileID by ... ? * decide about CustomizeGltfMaterial * adapt UCesium3DTilesetLifecycleEventReceiver's signatures to be implementable from a BP(??)

Fix missing condition on GetLifecycleEventReceiver() pointer

51d7b21

Reworked/simplified CreateMaterial/CustomizeMaterial

e30313e

revert adding CPU access option from Tilesets

dd2bf54

Merge branch 'main' into enh-1658_mesh-build-callbacks

e15b06a

Remove CPU access option mention from CHANGES.md

65efbd2

Merge remote-tracking branch 'cesium/main' into enh-1658_mesh-build-c…

40676e5

…allbacks

AdvViz: add GetVertexPositionScaleFactor to UCesiumLoadedTile to make…

5d357f5

… sense of glTF vertices

GhisBntly marked this pull request as ready for review June 11, 2025 14:27

GhisBntly added 3 commits June 17, 2025 13:19

CesiumMetadataValueAccess => FCesiumMetadataValueAccess

380fa13

missing fwd decl

335d612

Hide implementation detail

d58decc

* give access to Tileset actor

GhisBntly requested review from kring and j9liu June 17, 2025 11:50

j9liu added this to the August 2025 Release milestone Jun 30, 2025

kring reviewed Jul 22, 2025

View reviewed changes

Revert addition of tile render-readiness concept

e4336ac

GhisBntly added 2 commits July 22, 2025 15:27

Merge remote-tracking branch 'origin/main' into enh-1658_mesh-build-c…

cb8cade

…allbacks

Update CHANGES.md

c33861f

kring removed this from the August 2025 Release milestone Jul 31, 2025

Merge remote-tracking branch 'origin/main' into enh-1658_mesh-build-c…

8113a0a

…allbacks

GhisBntly force-pushed the enh-1658_mesh-build-callbacks branch from 0387394 to 8113a0a Compare August 14, 2025 11:51

GhisBntly added 10 commits August 14, 2025 15:25

Methods are no longer pure virtual, which is more user friendly

1293e31

language

e16a061

Revert "CesiumMetadataValueAccess => FCesiumMetadataValueAccess"

1adf464

This reverts commit 380fa13. I was supposed to remove the F from our version, not add it to the upstream... ;^^

Semantics and comments' rewrite by K.Ring

f2fd3a0

require that CreateMaterial returns non-null

67f880c

remove the need for GlTFmaterialPBR from CustomizeMaterial

ceb0511

and from SetGltfParameterValues, which it probably mimicked in the first place

bring back GetGltfModel, probably lost during a merge...

17c87f8

FindTexCoordIndexForGltfAttribute => FindTextureCoordinateIndexForGlt…

7c8b222

…fAccessor

doc comments in CesiumLoadedTile.h

ebd52b1

GhisBntly requested a review from kring August 14, 2025 15:16

[enhancement#1658] mesh, material and tile loading callbacks #1669

Are you sure you want to change the base?

[enhancement#1658] mesh, material and tile loading callbacks #1669

Uh oh!

Conversation

GhisBntly commented May 9, 2025

Uh oh!

j9liu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

j9liu May 9, 2025

Choose a reason for hiding this comment

Uh oh!

j9liu May 9, 2025

Choose a reason for hiding this comment

Uh oh!

kring May 12, 2025

Choose a reason for hiding this comment

Uh oh!

GhisBntly May 12, 2025

Choose a reason for hiding this comment

Uh oh!

kring May 16, 2025

Choose a reason for hiding this comment

Uh oh!

GhisBntly May 19, 2025

Choose a reason for hiding this comment

Uh oh!

kring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kring May 12, 2025

Choose a reason for hiding this comment

Uh oh!

GhisBntly commented Jun 11, 2025

Uh oh!

GhisBntly commented Jun 11, 2025

Uh oh!

GhisBntly commented Jun 12, 2025

Uh oh!

GhisBntly commented Jun 17, 2025

Uh oh!

j9liu commented Jul 8, 2025

Uh oh!

kring commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GhisBntly commented Jul 22, 2025

Uh oh!

GhisBntly commented Aug 14, 2025

Uh oh!

Uh oh!

kring commented Jul 22, 2025 •

edited

Loading