GLTF export improvements #91783
GLTF export improvements #91783
Conversation
ogapo
force-pushed
the
pr/gltf-export-fixes
branch
2 times, most recently
from
aeb354e to
8ce0310
Compare
|
GLTF export improvements on our list to review. |
There was a problem hiding this comment.
BUG: GLBs produced by Godot don't pass validation
Awesome, thanks for the fix. Wasn't aware of this before.
Remove empty "extensions" JSON object being appended to all nodes (if it's still empty).
I was aware of this but didn't get around to it, thank you for fixing this. I believe your fix is correct, since it preserves "extensions" when passing the JSON to GLTFDocumentExtension classes, and only removes it at the end.
Don't create an initial buffer until
Another great fix, I've seen this before but never got around to it. This adds more lines of code but it's worth it, because now it will exclude empty buffers. I wonder if this would affect GLTFDocumentExtension classes, though? Regardless I think it's good to have GLTFDocumentExtension classes check for a buffer before trying to write one, so I think your fix here is good.
Allow the case where root_nodes is empty.
I agree, this is allowed by the spec and should be allowed by Godot for non-scene glTF files. However note that the code in this PR is not sufficient to actually handle that case. If you export a scene with only the root node with MULTI_ROOT, there is currently a check for if (child_count > 0) { that will prevent MULTI_ROOT from being used in this case. However, removing that check is not sufficient by itself, because that causes the file to not be able to be imported back into Godot since it's not a scene, and also the "scene" and "scenes" would also need to be excluded. In order for the feature of non-scene glTF files to be made useful, we would need to implement godotengine/godot-proposals#7494 to allow importing glTF files as other resource types.
TL;DR: The changes in this PR are good, but the use case of empty root_nodes is not working yet. Further improvements can be left to another PR, this PR is good as-is.
- GLBs produced by godot don't pass validation when there's no data in the buffer segment. The segment is dropped but the size of the chunk_header is still reported in total length (incorrectly) - Remove empty "extensions" JSON object being appended to all nodes (if it's still empty). This is just cutting down on unnecessary bloat and consistent with the rest of the file's attempts to not emit any keys that are equal to their default value. - Allow the case where root_nodes is empty. This is permitted by the GLTF spec. Moreover it can happen fairly naturally when using the ROOT_NODE_MODE_MULTI_ROOT root node mode on a scene with only a root node (which is valid in godot). - Don't create an initial buffer until we're ready to write data into it (buffers of byteLength=0 don't pass validation).
ogapo
force-pushed
the
pr/gltf-export-fixes
branch
from
8ce0310 to
522f035
Compare
|
@fire good callout, I've adjusted the use of .size() as a boolean in the file @aaronfranke thanks for the review! Re: empty nodes, that makes sense. I didn't follow all the logic for MULTI_ROOT my actual case that lead to this change was generating the state manually and wanting to export it. I do wonder if removing the child count check you mentioned might still be worth doing in this PR though. I haven't included it yet, but wanted to talk it through. My understanding is that a scene which references no nodes is valid in the spec, so there wouldn't necessarily be any issue with the generated output. The fact that it wouldn't round trip is a good point, but there's lots of reasons one might use GLTF export for unidirectional workflows (mine is) and it seems like roundtrip support is why the SINGLE_ROOT extension exists anyway so maybe it's ok? |
I don't think so, because the output is not useful to be imported back into Godot yet, as Godot can't handle that situation at import time. I understand and agree that it has potential uses for other apps reading the glTF file (unidirectional workflows), but I would prefer to not expose a feature to users until it is working (or at least won't print error messages immediately after exporting the glTF file inside the Anyway, the point is that the import support should come "earlier than or at the same time as" the export support, which can potentially even be backported to allow older branches to import the files made in newer branches (in this case no, but in general it would be ideal).
GODOT_single_root exists to improve round-trip support, but regardless round-trip should still work somehow. |
|
Thanks! |
BUG: GLBs produced by Godot don't pass validation when there's no data in the buffer segment. The segment is dropped, but the size of the chunk_header is still reported in total length (incorrectly)
Remove empty "extensions" JSON object being appended to all nodes (if it's still empty). This is just cutting down on unnecessary bloat and consistent with the rest of the file's attempts to not emit any keys that are equal to their default value.
Allow the case where root_nodes is empty. This is permitted by the GLTF spec and it can happen fairly naturally when using ROOT_NODE_MODE_MULTI_ROOT on a scene with only a root node (which is a perfectly valid scene in Godot).
Don't create an initial buffer until we're ready to write data into it (buffers of byteLength=0 don't pass validation).