-
Notifications
You must be signed in to change notification settings - Fork 221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
convert I3dm metadata to EXT_structural_metadata and EXT_instance_features #1051
Conversation
The major refactor is to split the very rare case of EXT_mesh_gpu_instancing extension in the I3dm glTF into another function, and then reduce code duplication.
This class in PntsToGltfConverter.cpp is useful for dealing with instance metadata too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @timoore, this looks great! Just a few small comments below.
numInstances, | ||
type); | ||
gltf.accessors[static_cast<uint32_t>(accessorId)].byteOffset = | ||
static_cast<int64_t>(bufferOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this cast needed? It should be a widening conversion.
Also, for the one above, it's fine to use size_t(accessorId)
rather than the more verbose static cast when you know the accessorId can't be negative. If you're not sure of that, add a runtime check rather than a static_cast. If it can only be negative as a result of a developer error, add a CESIUM_ASSERT(accessorId >= 0);
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was getting a warning about unsigned to signed conversion, but I will check that.
Thanks @timoore! |
This mostly follows the lead of PntsToGltfConverter.cpp in converting the I3dm batch table to structural metadata and EXT_instance_features feature IDs. It does not handle the obscure case of an I3dm gltf model that already contains EXT_mesh_gpu_instancing instances. Tested with cesium-unreal and a tileset tiled from CZML.
Happy new year!