I3dm misc fixes #912
I3dm misc fixes #912
Conversation
The i3dm converter needs to know the coordinate axis convention of the tileset in order to correctly construct instance transformations. Resolves #904.
Calling GltfReader::resolveExternalData() during the i3dm conversion correctly handles relative path references in the .glb payload or in an external .glb file. Leaving it to Cesium3DTilesSelection::TilesetContentManager::loadTileContent(), where external data is resolved for normal 3D Tiles, gets relative paths wrong. Resolves #905.
|
@timoore this is segfaulting while running the tests, both on CI and on my own system. |
| // This use of a lambda is the only way timoore knows to conditionally pass | ||
| // different futures to a then...() member function. |
There was a problem hiding this comment.
I'd suggest something like this:
CesiumAsync::Future<GltfConverterResult> future =
header.gltfFormat == 0
? assetFetcher.get(baseUri).thenImmediately(
[options, assetFetcher](AssetFetcherResult&& assetFetcherResult)
-> CesiumAsync::Future<GltfConverterResult> {
if (assetFetcherResult.errorList.hasErrors()) {
GltfConverterResult errorResult;
errorResult.errors.merge(assetFetcherResult.errorList);
return assetFetcher.asyncSystem.createResolvedFuture(
std::move(errorResult));
}
return BinaryToGltfConverter::convert(
assetFetcherResult.bytes,
options,
assetFetcher);
})
: BinaryToGltfConverter::convert(gltfData, options, assetFetcher);Then on the next line:
return std::move(future).thenImmediately(...);That kind of intense ternary operator can be improved by moving the whole assetFetcher.get process to a separate function.
There was a problem hiding this comment.
I thought about using a ternary operator, but it's way less powerful than a lambda, being only an expression and all. Ultimately I didn't need to do anything too hairy inside this logic, but these days I kind of avoid the ternary operator in favor of lambda.
I had not thought about using std::move to call the next continuation "operator." I think we're getting off into matters of individual taste here; unless you feel strongly, I'm going to leave the current code as is.
There was a problem hiding this comment.
Well, I think continuation chains are a bit hard to read in the best of circumstances, and using a lambda here (we'd call this an IIFE - Immediately Invoked Function Expression - in JavaScript, but I don't know if C++ uses that term) makes it even more onerous than usual. clang-format really struggles with it. Another way that doesn't require a ternary or a separate function would be to use a std::optional to hold the Future, to work around the inability to default-construct one. Making Future default-constructible wouldn't be a terrible idea, either. A default-constructed Future would just be already rejected.
Anyway, I'm not strongly opposed to it as-is.
There was a problem hiding this comment.
I ended up keeping the lambda but separating the definition from the invocation. If nothing else, it reduces the indentation of the continuations that follow by a lot, which is a pretty significant win IMO.
The optional thing would work but felt hacky. Making Future default constructible isn't as easy as I hoped because it would still need an AsyncSystem. And so there would be a bit of cost to default-constructing a thing that would never be used.
gcc / clang is less tolerant than Visual Studio, which lets one leave off the trailing members of a braced initializer list.
This was a bug waiting to happen and should always have been a copy. Fortunately, the new use of the AssetFetcher to resolve external data for i3dm files provoked a crash in the tests.
Bingo. |
This whitespace may be added to keep the size of an i3dm file 8 byte aligned.
|
@timoore Let us know when this is ready for a re-review! |
It's ready for review. |
|
Assigning this back to @kring for last checks |
| @@ -44,7 +47,8 @@ struct CESIUM3DTILESCONTENT_API AssetFetcher { | |||
| const std::shared_ptr<CesiumAsync::IAssetAccessor> pAssetAccessor; | |||
| const std::string baseUrl; | |||
| glm::dmat4 tileTransform; // For ENU transforms in i3dm | |||
| const std::vector<CesiumAsync::IAssetAccessor::THeader>& requestHeaders; | |||
| const std::vector<CesiumAsync::IAssetAccessor::THeader> requestHeaders; | |||
There was a problem hiding this comment.
Now that requestHeaders is held by reference, asyncSystem should be too. Copying AsyncSystem is common and inexpensive (as the doc comments for the class explain).
There was a problem hiding this comment.
Also, all of these consts are unnecessary. The article Sean linked the other day does a pretty good job explaining why: https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/
| // order to make the size of the whole i3dm file 8-byte aligned. | ||
| auto rLastNotSpace = | ||
| std::find_if_not(gltfData.rbegin(), gltfData.rend(), [](auto&& b) { | ||
| return static_cast<int>(b) == ' '; |
There was a problem hiding this comment.
Casting to int and then comparing to a character? I think something like this would be clearer:
| return static_cast<int>(b) == ' '; | |
| return b == std::byte(' '); |
There was a problem hiding this comment.
I kind of had C on the brain, thinking that char promotes to int anyway. In the case that the glTF content is a URL with potential padding, it might make sense to create a subspan of char instead of std::byte.
| // This use of a lambda is the only way timoore knows to conditionally pass | ||
| // different futures to a then...() member function. |
There was a problem hiding this comment.
Well, I think continuation chains are a bit hard to read in the best of circumstances, and using a lambda here (we'd call this an IIFE - Immediately Invoked Function Expression - in JavaScript, but I don't know if C++ uses that term) makes it even more onerous than usual. clang-format really struggles with it. Another way that doesn't require a ternary or a separate function would be to use a std::optional to hold the Future, to work around the inability to default-construct one. Making Future default-constructible wouldn't be a terrible idea, either. A default-constructed Future would just be already rejected.
Anyway, I'm not strongly opposed to it as-is.
|
Thanks @timoore! |
This PR addresses two bugs first reported in the forum post https://community.cesium.com/t/i3dm-transformation-and-textures/32757 :
#904 - the tileset glTF up axis was not passed to the glTF converters, but i3dm translation depends on it;
#905 - the relative pathnames in external .glb files were not resolved correctly in the i3dm converter.
Fixes #904
Fixes #905
Fixes #893
Fixes #916