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: create separate accessors per group for non-indexed geometry #23333

Closed
wants to merge 5 commits into from

Conversation

jtbandes
Copy link

@jtbandes jtbandes commented Jan 25, 2022

Fixes #21538

Description

GLTFExporter handled groups for indexed geometry by adjusting the indices accessor. However, it was missing logic to handle groups in non-indexed geometry. This change adds support for non-indexed geometry by creating separate accessors for each group.

Also adds a new multi-colored cube example (both indexed and non-indexed) taken from #21538 to misc_exporter_gltf.html, and to the unit tests. Looks like the unit tests have been deleted 😓

image

One downside of this implementation (and the current exporter architecture) is that the attribute data gets duplicated when groups overlap. Fixing this seems like it might require a more difficult refactor and I would appreciate input from anyone familiar with the existing code.

@jtbandes
Copy link
Author

cc @takahirox @donmccurdy It looks like you both have worked on GLTFExporter; would you be able to review this change?

@mrdoob mrdoob added this to the r??? milestone Mar 1, 2022
@donmccurdy
Copy link
Collaborator

One downside of this implementation (and the current exporter architecture) is that the attribute data gets duplicated when groups overlap. Fixing this seems like it might require a more difficult refactor...

I don't think it's necessary to fix this – likely a rare case, and could be avoided by using indices. Performance-sensitive applications should probably be using indices anyway? Plenty of tools are available for optimizing the glTF after export as well.

This looks good to me if @takahirox has no concerns — thanks @jtbandes!

@jtbandes
Copy link
Author

Any update on this?

@takahirox
Copy link
Collaborator

Sorry, wanted to review but lack of time. Please ignore me for now because I don't want to block. I may review later as follow up.

@mrdoob
Copy link
Owner

mrdoob commented Jun 14, 2022

The thing is that I'm not really a fan of our geometry groups / multi material API anymore...

How does GLTF deal with the concept of geometry groups / multi material?

@takahirox
Copy link
Collaborator

takahirox commented Jun 14, 2022

How does GLTF deal with the concept of geometry groups / multi material?

In glTF a mesh can have multiple primitives.

https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#geometry

A primitive consists of attributes, indices, and material. A mesh having multiple primitives in glTF may correspond to a Mesh having geometry groups and multi materials in Three.js if the attributes in multiple primitives refer to the same accessor sets.

{
    "meshes": [
        {
            "primitives": [
                {
                    "attributes": {
                        "POSITION": 0,
                        "NORMAL": 1,
                        "TEXCOORD_0": 2
                    },
                    "indices": 3,
                    "material": 0,
                    "mode": 0
                },
                {
                    // Referring to the same accessors in attributes
                    "attributes": {
                        "POSITION": 0,
                        "NORMAL": 1,
                        "TEXCOORD_0": 2
                    },
                    // But referring to different indices accessor and material
                    "indices": 4,
                    "material": 1,
                    "mode": 0
                }
            ]
        }
    ]
}

Three.js GLTFLoader used to create a Mesh having geometry groups and multi materials from a mesh having multiple primitives referring to the same accessor sets from attributes in glTF before.

But we realized that geometry groups and multi materials can make GLTFLoader complex. So we decided to create a Group having multiple Meshes from it instead. So now GLTFLoader doesn't generate geometry groups and multi materials.

@donmccurdy
Copy link
Collaborator

Each glTF mesh could contain many primitives, where some primitives are triangle meshes and others are points or lines, they do not have to be consistent. So, mapping a mesh to THREE.Group containing {Mesh | Points | Lines | ...}[] in three.js works well enough, but the relationship might be too loose to be useful for inspiring a new three.js object API.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 30, 2023

Closing in favor of #27268.

@Mugen87 Mugen87 closed this Nov 30, 2023
@Mugen87 Mugen87 removed this from the r??? milestone Nov 30, 2023
@kolodi
Copy link
Contributor

kolodi commented Apr 3, 2024

Splitting a single mesh with multi materials (geometry groups) into separate meshes during export/import feels not quite right. It breaks many workflows dependant on nodes structure of the scene. In my case, I really need single mesh for the decals to work properly. Once you have exported a single mesh into multiple glTF primitives there must be a way to restore the same exact single mesh during the import process.
To work around this issue I have introduced the automerge step after the import based on some userData information in my specific application, but this adds a certain overhead to the loading process. I think restoring original scene shape directly during export/import is a more correct approach.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Apr 3, 2024

I've given up on trying to have GLTFLoader create multi-material meshes during the loading process — there's no possible glTF input for which GLTFLoader will currently construct a multi-material mesh. Trying to do so within the loader added a lot of complexity and overhead in the past, so instead we produce a THREE.Group containing a THREE.Mesh for each glTF mesh primitive.

I have no strong preference on what GLTFExporter should do, though.

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 does not honor groups with non-indexed geometry.
6 participants