Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GLTFExporter: Missing required padding on integer vertex attributes #27064

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

grzesiekmq
Copy link
Contributor

@grzesiekmq grzesiekmq commented Oct 26, 2023

Fixed #25309

Description

Problem was with missing padding on integer vertex attributes.
Problem solved by padding with additional two 0's to 6 bytes filled Uint8Array , to make it 8 bytes, so
result is Uint8Array([0, 0, 1, 0, 0, 1, 0, 0])
like on screenshot below:
image

But note issue with required stride is not still solved

@mrdoob mrdoob changed the title Missing required padding on integer vertex attributes GLTFExporter: Missing required padding on integer vertex attributes Oct 26, 2023
@grzesiekmq grzesiekmq marked this pull request as draft October 26, 2023 14:08
@grzesiekmq grzesiekmq marked this pull request as ready for review October 26, 2023 14:10
@grzesiekmq grzesiekmq marked this pull request as draft October 28, 2023 08:17
@grzesiekmq grzesiekmq marked this pull request as ready for review October 28, 2023 09:50
Repository owner deleted a comment from jabri62018 Nov 10, 2023
@mrdoob
Copy link
Owner

mrdoob commented Nov 14, 2023

@donmccurdy Is this required?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Nov 16, 2023

I agree that something like this is probably needed. However, I'm not sure if the padding should be limited to cases where target === WEBGL_CONSTANTS.ARRAY_BUFFER, and I believe padding may also be needed when componentSize is 2. And as @grzesiekmq mentions, there is a remaining issue with stride.

Right now I don't have time to investigate or test this, but if someone else can, please do!

This is a case where I do wish we still had unit tests covering GLTFExporter, it would save a lot of time manually checking edge cases.

@takahirox
Copy link
Collaborator

takahirox commented Feb 15, 2024

Hi, thanks for making a PR!

I don't think I have looked into the issue and the change in deep yet but let me share my some impressions for now.

if(componentSize === 1){
  const paddedArray = new Uint8Array(getPaddedArrayBuffer(attribute.array.buffer));
  attribute.array = paddedArray;
}

It's a bit weird to me that Exporter has a side effect to Three.js objects. Ideally no side effect in Three.js objects, but adjusting the byte offset and stride only in exported glTF would be better.

Also, wondering whether it's a good place to insert the code. The code is inserted at almost the end of processBufferView() and after that part that attribute doesn't seem to be accessed.

As @donmccurdy 's comment,

I believe padding may also be needed when componentSize is 2.

I agree with it.

This is a case where I do wish we still had unit tests covering GLTFExporter, it would save a lot of time manually checking edge cases.

I feel the same. glTF loader and exporter are under addons but because of the standard 3D format they are ones of the most popular modules in Three.js and I have been wanting automatic/unit tests for them. If lack of resources for maintaining the tests for glTF stuffs in the official Three.js repo, I've been thinking of that we volunteers make an external Three.js glTF stuffs test repository, that has glTF loader/exporter unit/e2e tests, imports Three.js glTF loader/exporter, and run the tests.

@takahirox
Copy link
Collaborator

If I understand the issue correctly, glTF loader already attempts to keep byte offset on four-byte boundary but doesn't take care of byte stride (as described in #25309).

Perhaps the lines we need to edit are here. I think we need to

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2024

@grzesiekmq Let me know if you are too busy now. I can take over because this issue really needs to be fixed. Even if I do I ask @Mugen87 to add your name to the release log because I want to respect your original work.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2024

@takahirox Feel free to take over and update this PR! 👍

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2024

OK. I would like to wait for @grzesiekmq 's response for a few days for now because I really don't want to steal or interrupt the contribution. And then I would try to update this PR if no response.

@grzesiekmq
Copy link
Contributor Author

@grzesiekmq Let me know if you are too busy now. I can take over because this issue really needs to be fixed. Even if I do I ask @Mugen87 to add your name to the release log because I want to respect your original work.

Yes, please take it over, I don't have idea how to continue this

@takahirox
Copy link
Collaborator

takahirox commented Feb 29, 2024

OK, I will take it over. I'm working on the issue...

WIP: https://github.com/takahirox/three.js/blob/FixGLTFExporterPadding/examples/jsm/exporters/GLTFExporter.js

@takahirox
Copy link
Collaborator

takahirox commented Feb 29, 2024

@donmccurdy

However, I'm not sure if the padding should be limited to cases where target === WEBGL_CONSTANTS.ARRAY_BUFFER

I think this padding should be limited to cases where target === WEBGL_CONSTANTS.ARRAY_BUFFER because of these descriptions in the spec

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#buffers-and-buffer-views-overview

When a buffer view is used for vertex attribute data, it MAY have a byteStride property. This property defines the stride in bytes between each vertex. Buffer views with other types of data MUST NOT not define byteStride (unless such layout is explicitly enabled by an extension).

and

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#accessors-overview

The byteOffset property specifies the location of the first data element within the referenced buffer view. If the accessor is used for vertex attributes (i.e., it is referenced by a mesh primitive or its morph targets), the locations of the subsequent data elements are controlled by the bufferView.byteStride property. If the accessor is used for any other kind of data (vertex indices, animation keyframes, etc.), its data elements are tightly packed.

@donmccurdy
Copy link
Collaborator

@takahirox ah you're right, thank you!

@takahirox
Copy link
Collaborator

I have reflected this idea to this PR, and confirmed that the error is disappeared in this example.

I didn't test many enough yet, but if you folks start to review, it's very helpful to me.

@takahirox
Copy link
Collaborator

takahirox commented Mar 16, 2024

I think ready for review @donmccurdy @Mugen87

Actually, I exported scene1 from https://threejs.org/examples/#misc_exporter_gltf and loaded in https://gltf-viewer.donmccurdy.com/ . These validation errors are disappeared with the change.

image

Copy link
Collaborator

@donmccurdy donmccurdy left a comment

Choose a reason for hiding this comment

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

Thank you!

@donmccurdy donmccurdy merged commit a7712cc into mrdoob:dev Mar 19, 2024
11 checks passed
@Mugen87 Mugen87 modified the milestones: r164, r163 Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GLTFExporter: Missing required padding on integer vertex attributes
5 participants