GLTF: Move accessor encoding functions to GLTFAccessor#108853
Merged
Repiteo merged 1 commit intogodotengine:masterfrom Nov 13, 2025
Merged
GLTF: Move accessor encoding functions to GLTFAccessor#108853Repiteo merged 1 commit intogodotengine:masterfrom
Repiteo merged 1 commit intogodotengine:masterfrom
Conversation
069f28f to
8d2c4a6
Compare
Member
|
When the pr is out of draft state I'll take a look. what is missing? |
8d2c4a6 to
7d9b6db
Compare
7d9b6db to
27066a7
Compare
27066a7 to
9571a64
Compare
9571a64 to
d5ab2a3
Compare
This was referenced Oct 13, 2025
d5ab2a3 to
62acc21
Compare
fire
approved these changes
Oct 21, 2025
Member
fire
left a comment
There was a problem hiding this comment.
Ok with this in general as long as the gltf i/o still works. Did not review deeply and test with gltf test cases.
Contributor
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR moves all of the accessor encoding functions currently found in GLTFDocument into the GLTFAccessor class. The goal here is to allow for these functions to be used outside of GLTFDocument, such as by extensions, so that extensions can read and write accessor data in a type-safe way.
This is a prerequisite PR to another prerequisite PR to allow for MultiMeshInstance3D import via EXT_mesh_gpu_instancing as seen in PR #107866. After the accessor encoding functions, I need to also move the decoding functions. Getting so many details right with the encoding functions, including comparing the glTF specification with the current Godot implementation and making sure this code works correctly with all the quirks and edge cases... took a lot of effort, so I don't want to do the decoding functions in the same PR.
Now, I said "move", but this PR is far from a trivial move. This is a refactor where I combed over the code multiple times to ensure that we actually have a good API free from bugs, and has deduplication built-in (this needs to be done before exposing the API signature because there's a boolean parameter to disable it). Of course, it's possible that I've missed something, so this should be ideally be tested with exporting many models before merging.
Here's a list of changes, as comprehensive as I can make it:
_from_variantsfunction to encode any Array, or use the low-level API._encode_accessor_into_buffer_viewreplaced withencode_floats_as_bytes,store_accessor_data_into_state, and_determine_skip.encode_ints_as_bytesand corresponding high-level functions. Fixes the ability for extensions to use signed/unsigned long component types.stride += 4 - (stride % 4); //according to spec must be multiple of 4in_encode_accessor_into_buffer_viewwas setting the byte stride, but the rest of the logic was always packing the data tightly. It's not possible for the existing export code to trigger this problem since this line of code in master is never run, but if it did run, the data would be malformed, which would be a bug. With exposing this to the user, it's possible that a user could make this mistake. I made the code throw an error if the user tries to export a misaligned vertex attribute accessor, so that the user can fix their export code, instead of the exporter lying about the byte stride.p_for_vertexboolean parameters with theGLTFBufferView::ArrayBufferTargetenum, making it clear what value is being passed._filter_numberis replaced with_filter_numbers, checking all the items in the array at once instead of having to call a function for every single item. It now only clamps to single-precision float when the component type is single float - this fixes a bug in master where the glTF module would break "unsigned integer" components above 16.7 million, and also fixes a potential future bug if users wanted to use the double or long component types._calc_accessor_min_maxand_round_min_max_componentscombined into_calculate_min_and_max. Like filter number, loops within the function instead of calling for every item. Like filter number, only casts to single-precision float when the component type is single float.Vector4instead ofColor(they're... not colors).Vector4iinstead ofColor(they're... also not colors).GLTFAccessor::_get_indices_component_type_for_size)._encode_sparse_accessor_as_vec3is replaced withencode_new_sparse_accessor_from_vec3s. Fix a bug wherep_reference_multiplierwas not working correctly when the base reference array had contents in it. Note that this was never used, the base reference array is always empty in all uses in master..gltfis 137806 bytes and the.binis 1146484 bytes. With deduplication, the.gltfis 101443 bytes (26% reduction) and the.binis 750728 bytes (34% reduction). This can save potentially lots of space, or little, it depends on the model. The code is very careful to only ever deduplicate when it's an exact duplicate, including all the properties matching, and is compliant with the glTF specification saying that buffer views with different targets must be kept separate (because differing targets will make them ineligible for deduplication with this code).p_reference_accessorin the old_encode_sparse_accessor_as_vec3, since it considers everything for deduplication, not just what's referenced by that one accessor. However, note that this existingp_reference_accessorfeature is never used, it's always -1 in all uses in master.TL;DR: Aside from the deduplication, fixing several edge cases (Ctrl+F for "bug"), and minor improvements like unsigned byte, this PR should have the same behavior as on master, and of course, should work correctly for all cases which already work correctly in master. Testing is appreciated, though, and should be done before merging.