GLTF Serializer rework. #15869
GLTF Serializer rework. #15869
Conversation
…into sergio/new-gltf-exporter-2
…into sergio/new-gltf-exporter-2
packages/dev/serializers/src/glTF/2.0/glTFMorphTargetsUtilities.ts
Outdated
Show resolved
Hide resolved
packages/dev/serializers/src/glTF/2.0/glTFMorphTargetsUtilities.ts
Outdated
Show resolved
Hide resolved
packages/dev/serializers/test/integration/glTFSerializer.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Gary Hsu <[email protected]>
Co-authored-by: Gary Hsu <[email protected]>
Co-authored-by: Gary Hsu <[email protected]>
…oRZMasson/Babylon.js into sergio/new-gltf-exporter-2
|
I'm temporarily converting this back to draft since we are waiting on @ryantrem, the plan is to merge it next week. |
|
Any considerations on keeping the original texture image instead of re-encoding it? |
Hi, is there any progress. I see that the merger has been delayed for a long time |
|
@Jeggery, please be mindful of everyone's time. It has only been delayed a week due to vacation... If you need something faster, this is open source, feel free to make a PR for it. |
|
@sebavan OK, sorry I didn't know you guys were on holiday, I was looking forward to it |
|
nw, the team is fully back this week |
The PR is already very big, we plan to merge the changes we have so far and follow it up with smaller improvements. Maybe this change can be added in a following PR. |
| } | ||
| break; | ||
| } | ||
| case AccessorComponentType.BYTE: { | ||
| for (let i = 0; i != buffer.length; i++) { | ||
| binaryWriter.setByte(buffer[i] * 127); | ||
| binaryWriter.writeUInt8(buffer[i] * 127); |
| // glTF lights have variable rotation and constant direction. Therefore, | ||
| // compute a quaternion that aligns the Babylon light's direction with glTF's constant one. | ||
| if (lightType !== KHRLightsPunctual_LightType.POINT) { | ||
| const direction = babylonNode.direction.normalize(); |
There was a problem hiding this comment.
normalize normalizes in place, so babylonNode.direction will be normalized: I'm not sure it's expected? You can use normalizeToNew() to create a new normalized vector.
| private _checkGrowBuffer(byteLength: number): void { | ||
| const newByteLength = this.byteOffset + byteLength; | ||
| if (newByteLength > this._data.byteLength) { | ||
| const newData = new Uint8Array(newByteLength * 2); |
There was a problem hiding this comment.
Depending on the size of the DataWriter buffer / the number of DataWriter instances in memory at the same time, we could consume much more memory than necessary with this scheme of doubling the buffer size when it's too small. I assume that getOutputData is called when we've finished filling the buffer? In that case, you could recreate the internal buffer at the final size and not waste memory.
If there is only one instance of DataWriter (for the entire serialized scene), this can be even more problematic, as some glb files can be quite large, and, on average, the data buffer will be 50% larger than necessary.
Perhaps it would be better simply to add a fixed constant value?
| map3.set(offset, map4); | ||
| } | ||
|
|
||
| map4.set(flip, accessorIndex); |
There was a problem hiding this comment.
Is it possible that an entry already exists for flip in map4? If so, is it a problem / an error? Same question for the other methods below that are implemented in the same way.
| } | ||
|
|
||
| /** | ||
| * Rotation by 180 as glTF has a different convention than Babylon. |
There was a problem hiding this comment.
I think that if our conversion from LH to RH consisted simply of negating Z (and not X), we wouldn't need this conversion, because the difference between the glTF coordinate system and Babylon is that the Z axis is reversed (see https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#coordinate-system-and-units).
| parentMatrix.multiplyToRef(matrix, matrix); | ||
| matrix.decompose(parentScale, parentRotation, parentTranslation); | ||
|
|
||
| if (parentTranslation.equalsToFloats(0, 0, 0)) { |
There was a problem hiding this comment.
Shouldn't equalsWithEpsilon be used instead? There are other occurrences in this and other files that could benefit from the use of equalsWithEpsilon to take floating-point calculation errors into account.
| * @returns True if the parent node was added by the GLTF importer. | ||
| */ | ||
| export function IsParentAddedByImporter(babylonNode: Node, parentBabylonNode: Node): boolean { | ||
| return parentBabylonNode instanceof TransformNode && parentBabylonNode.getChildren().length == 1 && babylonNode.getChildren().length == 0; |
There was a problem hiding this comment.
I think it would be more robust for the loader to add metadata specific to the node it creates, which we could check afterwards, as I think the above condition could also apply to ordinary user-created nodes?
| } | ||
|
|
||
| export function IsNoopNode(node: Node, useRightHandedSystem: boolean): boolean { | ||
| if (!(node instanceof TransformNode)) { |
There was a problem hiding this comment.
Same comment as above. It would be more robust if the loader added specific metadata. This would also allow us to handle the case where the user updates this node (which has already happened in PGs), meaning we can no longer check if the matrix is the identity matrix to determine if the node is a “noop” node.
Ditto. I can look into using original textures if they're available, but it'll have to be after this PR. Thanks for bringing this up! :) |
Co-authored-by: Popov72 <[email protected]>
Co-authored-by: Popov72 <[email protected]>
Co-authored-by: Popov72 <[email protected]>
Co-authored-by: Popov72 <[email protected]>
|
Visualization tests for WebGPU (Experimental) |
This PR changes fundamentally how the Babylon serializer to GLTF works. It changes the way the serializer handles left hand to right hand conversion.
Left hand to Right hand conversion
We no longer set mesh nodes with a negative scaling to handle left vs right hand conversion. We instead change every single vertex information when needed (normal, positions, and tangents) if the mesh was created in a left handed scene. We do the same for animation data and node transforms.
This should help decrease the file size of the exported glTF by removing the need for a root node to convert handedness, essentially reverting #13909.
Cameras and Lights
We now collapse cameras and lights with their transform nodes that are added by the Babylon importer. The goal is for the scene to be able to roundtrip without infinitely expanding.
Preserve vertex data
We now avoid as much as possible changing the type of vertex data. If the Babylon scene is holding vertex data using a particular type we will avoid at all cost converting it to float or to any other type. We might apply transformations to the data (such as left to right hand system conversion) but those modifications will be done in place, avoiding as much as possible creating a new buffer.
We also allow meshes that share the same geometry to point to the same accessors in the generated GLTF, se we no longer do any buffer duplication.
This should help significantly decrease some glTF file sizes on roundtrip, as we no longer make copies of buffers nor needlessly convert smaller buffer types to float buffers.
Forum issues
This PR also fixes a couple of bugs from the forum:
Wrong flipping of animated cameras
Inconsistent transform serialization
Instances exporting as separate meshes and #14056