GLTF: Move accessor decoding functions to GLTFAccessor#109103
Merged
Conversation
5da2048 to
24463ab
Compare
24463ab to
80f4298
Compare
80f4298 to
dbbeab7
Compare
dbbeab7 to
91acff8
Compare
This was referenced Oct 27, 2025
Member
|
I think we should merge this for 4.6, but what is the correct order of operations? |
Member
Author
|
@fire On every one of my draft PRs, the very top of the description has links to all the dependency PRs. |
Member
|
It's blocked on #108853 |
91acff8 to
04aa8d4
Compare
fire
reviewed
Nov 14, 2025
04aa8d4 to
d97d9f7
Compare
fire
approved these changes
Nov 14, 2025
Repiteo
reviewed
Nov 14, 2025
49ee683 to
9d0b391
Compare
Repiteo
approved these changes
Nov 14, 2025
Contributor
|
Thanks! |
Member
|
@akien-mga bisected a [gltf import] 1x to 2x slowdown because of this pull request on master. I'll post the most current master commit 4601c07 but the git bisect was done earlier. aaronfranke is checking now. |
Member
Author
|
For the record: The specific part of the code that regressed in performance was about 50 times slower. So it was catastrophically bad, apologies for my mistake. Anyway, it is now fixed as of PR #113172. |
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.
Draft because this depends on #108853.
This PR moves the logic of the accessor decoding 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 allow for MultiMeshInstance3D import via EXT_mesh_gpu_instancing as seen in PR #107866.
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. Of course, it's possible that I've missed something, so this should be ideally be tested with importing many models before merging.
Here's a list of changes:
GLTFDocument::_decode_buffer_view(really accessor logic) replaced withGLTFAccessor::_decode_raw_numbersGLTFDocument::_decode_accessorreplaced withGLTFAccessor::_decode_as_numbersstride += 4 - (stride % 4); //according to spec must be multiple of 4was bogus in the decoding logic, for the same reasons as it was in the encoding logic. Yes, the spec says it must be a multiple of 4... but if the files has the wrong stride, we can't just pretend it has a different one. Instead of "correcting", we need to error if this happens. With this PR, such invalid cases will print"glTF import: The declared buffer view byte stride " + itos(declared_byte_stride) + " was not a multiple of 4 as required by glTF.". Removing this also means we can drop thep_for_vertexparameter.I tested this PR on real 3D models and they work correctly, and also stress-tested it on these test files which are specifically designed to contain all the edge cases of glTF and it works, but more testing is appreciated.