cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

Support 3D Tile I3dm legacy tile format

Open timoore opened this issue 1 year ago • 6 comments

Initial commit for discussion. Some implementation remains, such as implementation of the ENU rotation option.

I3dm strains the GltfConverters interface because it can require a "sub read" of external content: the instanced glb file can be specified by a URI. I created a "ConverterSubprocessor" subclass (pretty bad name) to pass, the async system asset Accessor, base URI, etc. GltfConverters::convert returns a complete model, not a future, so I do a wait() on the accessor future that returns the .glb file. I don't know if that's bad form or if there's a better alternative.

This addresses #458.

timoore avatar Apr 08 '24 16:04 timoore

Haven't dug in real deep yet, but noticed there are no new unit tests .

Might be nice to add a few, especially if you know of typical use cases, or cases that should fail for some reason.

csciguy8 avatar Apr 08 '24 19:04 csciguy8

GltfConverters::convert returns a complete model, not a future, so I do a wait() on the accessor future that returns the .glb file. I don't know if that's bad form or if there's a better alternative.

Yeah adding a wait is pretty bad. In Unreal, for example, that'll block up Unreal's task graph on a network request, which in extreme cases could completely pause rendering.

Based on a quick look, I don't think it will be too big a change to make GltfConverters::convert return a Future. If it is, though, another possibility is to convert the i3dm to glTF synchronously and include references to the external resources in that glTF (using a private extension if needed). Then do the async portion later. If you happen to be able to hook into the existing GltfReader::resolveExternalData, the extra async step will just happen automatically. If not, you'll need to add another processing step near where resolveExternalData is located in order to do this extra async work.

kring avatar Apr 10 '24 21:04 kring

GltfConverters::convert returns a complete model, not a future, so I do a wait() on the accessor future that returns the .glb file. I don't know if that's bad form or if there's a better alternative.

Agree with @kring , hidden waits aren't great when trying to improve performance.

Will also chime in that hidden IAssetAccessor::gets can become problematic too, especially when trying to manage simultaneous connections to a server, or decouple their execution from other futures that are just CPU processing work. This is the bulk of the work in this Staged Loading PR, and it's not a trivial refactor.

csciguy8 avatar Apr 11 '24 18:04 csciguy8

GltfConverters::convert returns a complete model, not a future, so I do a wait() on the accessor future that returns the .glb file. I don't know if that's bad form or if there's a better alternative.

Agree with @kring , hidden waits aren't great when trying to improve performance.

I'm rewriting the converter code to return a future.

Will also chime in that hidden IAssetAccessor::gets can become problematic too, especially when trying to manage simultaneous connections to a server, or decouple their execution from other futures that are just CPU processing work. This is the bulk of the work in this Staged Loading PR, and it's not a trivial refactor.

What makes an IAssetAccessor::get call hidden? Or, put another way, what's the alternative when another get() needs to be made in the while processing the content of a previous read? I guess I should look at #779 to see what you're doing there.

timoore avatar Apr 12 '24 09:04 timoore

What makes an IAssetAccessor::get call hidden? Or, put another way, what's the alternative when another get() needs to be made in the while processing the content of a previous read? I guess I should look at #779 to see what you're doing there.

That's a fair question. In #779, a good example of the approach is here in ImplicitOctreeLoader

  • The caller would typically call ::loadTileContent, which returns a future, and somewhere in the chain is a IAssetAccessor::get
  • This PR reworks the approach to have the caller use ::getLoadWork, which returns the needed request and the CPU processing work separately
  • This pain is in the rework of the code in the .cpp, and not break anything

Admittedly, this is "a" solution to separating asset request work from other work. This PR hasn't been reviewed or merged, so there may be a better solution still.

csciguy8 avatar Apr 12 '24 21:04 csciguy8

@timoore haven't forgotten about this PR, I'll plan to take another look tomorrow :pray:

j9liu avatar May 20 '24 19:05 j9liu

Thanks for addressing all the feedback @timoore ! Sorry I didn't see this earlier, you're welcome to @ me after you push changes to get my attention :smile:

Merging now!

j9liu avatar May 29 '24 13:05 j9liu