X Tutup
Skip to content

GLTF: Move accessor encoding functions to GLTFAccessor#108853

Merged
Repiteo merged 1 commit intogodotengine:masterfrom
aaronfranke:gltf-accessor
Nov 13, 2025
Merged

GLTF: Move accessor encoding functions to GLTFAccessor#108853
Repiteo merged 1 commit intogodotengine:masterfrom
aaronfranke:gltf-accessor

Conversation

@aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 22, 2025

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:

  • Accessor encoding logic is now in GLTFAccessor instead of GLTFDocument. GLTFDocument has ~1100 fewer lines of code in it, GLTFAccessor gains ~800 lines.
  • Significantly less code duplication. More small functions, instead of big monolithic functions with repeated internals.
  • Generally more comments and more helpful error messages.
  • Low-level API for fine control, high-level API for typical use cases.
  • High-level API functions are determined by the existing needs of GLTFDocument, without speculating on future use cases. User code can always use the _from_variants function to encode any Array, or use the low-level API.
  • _encode_accessor_into_buffer_view replaced with encode_floats_as_bytes, store_accessor_data_into_state, and _determine_skip.
  • Dedicated integer encoding functions with encode_ints_as_bytes and corresponding high-level functions. Fixes the ability for extensions to use signed/unsigned long component types.
  • This line stride += 4 - (stride % 4); //according to spec must be multiple of 4 in _encode_accessor_into_buffer_view was 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.
  • Replaced the p_for_vertex boolean parameters with the GLTFBufferView::ArrayBufferTarget enum, making it clear what value is being passed.
  • _filter_number is 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_max and _round_min_max_components combined 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.
  • Weights now use Vector4 instead of Color (they're... not colors).
  • Joints now use Vector4i instead of Color (they're... also not colors).
  • Joints can now use unsigned byte components instead of unsigned short if the values are all low enough, saving some space, as allowed by the glTF specification and supported by the import code.
  • Integers and sparse indices are now allowed to be unsigned byte if less than 255. Previously it only checked > 65535 and used unsigned int, or else unsigned short.
    • Yes, less than 255, not 255 or less. The index 255 in unsigned byte, and 65535 in unsigned short, and 4294967295 in unsigned int, are reserved by the glTF specification as primitive restart values ("3.7.2.1. indices accessor MUST NOT contain the maximum possible value for the component type used (i.e., 255 for unsigned bytes, 65535 for unsigned shorts, 4294967295 for unsigned ints)."). This means that there was a bug in master: if some indexed data happened to go up to index 65535 but no higher, then Godot would use unsigned short, resulting in invalid data. So, the new threshold is one less, fixing the problem (see the new GLTFAccessor::_get_indices_component_type_for_size).
  • _encode_sparse_accessor_as_vec3 is replaced with encode_new_sparse_accessor_from_vec3s. Fix a bug where p_reference_multiplier was 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.
  • Sparse accessors take into account the amount of extra bytes the JSON data will take up when calculating if it's worth using a sparse accessor vs a dense accessor.
  • New feature: Deduplication. On the "AnimateAllTheThings" test asset from Khronos, round-tripping in and out of Godot, without deduplication the .gltf is 137806 bytes and the .bin is 1146484 bytes. With deduplication, the .gltf is 101443 bytes (26% reduction) and the .bin is 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).
    • Also note, the existence of deduplication replaces the need for p_reference_accessor in 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 existing p_reference_accessor feature 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.

@fire
Copy link
Member

fire commented Jul 23, 2025

When the pr is out of draft state I'll take a look. what is missing?

@aaronfranke aaronfranke marked this pull request as ready for review October 21, 2025 16:50
@aaronfranke aaronfranke requested a review from a team as a code owner October 21, 2025 16:50
@aaronfranke aaronfranke moved this from Work in progress to Ready for review in Asset Pipeline Issue Triage Oct 21, 2025
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok with this in general as long as the gltf i/o still works. Did not review deeply and test with gltf test cases.

@Repiteo Repiteo merged commit 12cdb66 into godotengine:master Nov 13, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in Asset Pipeline Issue Triage Nov 13, 2025
@Repiteo
Copy link
Contributor

Repiteo commented Nov 13, 2025

Thanks!

@aaronfranke aaronfranke deleted the gltf-accessor branch November 14, 2025 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

3 participants

X Tutup