CesiumGS / cesium-native

Apache License 2.0
402 stars 205 forks source link

Support 3D Tile I3dm legacy tile format #854

Closed timoore closed 2 months ago

timoore commented 4 months ago

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.

csciguy8 commented 4 months ago

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.

kring commented 4 months ago

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.

csciguy8 commented 4 months ago

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.

timoore commented 4 months ago

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.

csciguy8 commented 4 months ago

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

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.

j9liu commented 3 months ago

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

j9liu commented 2 months ago

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!