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

GLTF Loader #732

Merged
merged 11 commits into from
Apr 18, 2024
Merged

GLTF Loader #732

merged 11 commits into from
Apr 18, 2024

Conversation

panxinmiao
Copy link
Contributor

@panxinmiao panxinmiao commented Apr 16, 2024

GLTF file loader for pygfx, completely based on the GLTF 2.0 specification.

This PR has been separated out from #715 . It is the prerequisite for completing #715 .

It implements the basic parsing logic for the GLTF file data (base on gltflib), and has also completed the loading of Mesh (including Geometry and Material).

I hope that it will ultimately be fully support the GLTF 2.0 specification, and can also be used as a reference for the design of the capability API for the future development of pygfx.

@panxinmiao panxinmiao requested a review from Korijn as a code owner April 16, 2024 07:53
@almarklein
Copy link
Collaborator

almarklein commented Apr 16, 2024

Super valuable contribution, thanks!

We now support gltf via trimesh. I presume you usef gltflib because the gltf support in trimesh is limited? Does this code-path support more of gltf or are there also things that trimesh supports, which are not yet in this code here?

API-wise, I'm not a big fan of the capitalized GLTF. I wonder if we should do something like this:

  • In load_trimesh.py implement load_mesh_trimesh() and load_scene_trimesh()
  • In load_gltf.py implement load_mesh_gltf() and load_scene_gltf()
  • In load.pyimplement load_mesh() and load_scene() which call out to one of the above, based on the extension.

edit: that refactoring is perhaps a bit more than you originally planned, but we can add load_mesh_gltf() and work towards the above later.

@panxinmiao
Copy link
Contributor Author

We now support gltf via trimesh. I presume you usef gltflib because the gltf support in trimesh is limited?

Yes, the trimesh library has very limited support for GLTF.

The GLTF specification is essentially defined using JSON to describe the scene content, along with embedded or external resource files. GLB, on the other hand, is a format that packages the JSON definition and various resource files into a single file with a specific structure. The gltflib library is a basic reading and writing library for GLTF/GLB files, and it does not involve any detailed parsing. Through the gltflib library, we can conveniently and quickly access any content we want from the GLTF/GLB files.

Does this code-path support more of gltf or are there also things that trimesh supports, which are not yet in this code here?

Therefore, whether this code-path supports more GLTF features completely depends on how much we have implemented.

@panxinmiao
Copy link
Contributor Author

API-wise, I'm not a big fan of the capitalized GLTF. I wonder if we should do something like this:

I have designed the GLTF loader as a class rather than a function here because there are many intermediate states that need to be maintained during the loading and conversion process, and using a class can make it simpler and more clear.

Perhaps I could make it an internal class object and then provide a unified function interface?

@almarklein
Copy link
Collaborator

Perhaps I could make it an internal class object and then provide a unified function interface?

Yes, I think the user-facing API is easier if its just a function (or two). Thanks!

@panxinmiao
Copy link
Contributor Author

Does this code-path support more of gltf or are there also things that trimesh supports, which are not yet in this code here?

I believe that the current PR already covers all the functionality that using trimesh to load GLTF (with a bit more, such as the second set of texture coordinates). I will continue to expand its support for other aspects of GLTF (synchronizing with the capabilities of pygfx).

However, it is only effective for the GLTF/GLB format, as trimesh also supports some other model file formats.

Nevertheless, the GLTF format is relatively universal, and there are many third-party libraries and tools that support converting various model types to the GLTF format. By supporting GLTF well, we can effectively support the vast majority of model file formats.

@panxinmiao
Copy link
Contributor Author

panxinmiao commented Apr 16, 2024

  • In load_trimesh.py implement load_mesh_trimesh() and load_scene_trimesh()
  • In load_gltf.py implement load_mesh_gltf() and load_scene_gltf()
  • In load.pyimplement load_mesh() and load_scene() which call out to one of the above, based on the extension.

I have refactored the code.

I think that the name load_scene() is not very suitable for GLTF, as this function does not just load a single scene. The file may contain multiple scenes, as well as other content such as animations. Therefore, I have used the name load_gltf() instead.

What do you think? @almarklein

@panxinmiao
Copy link
Contributor Author

panxinmiao commented Apr 16, 2024

Additionally, the behavior of load_gltf_mesh() and the implementation using trimesh (load_mesh) are slightly different. The load_gltf_mesh() method will only return a list of Mesh objects, without any additional information, such as Skeleton or Transform. This is because this method only reads the Mesh definition part of the GLTF file and does not parse the entire scene node graph. If the user wants to obtain the Transform information, they should use the load_gltf() method to get the entire scene.

To add a bit more context, when users use the load_gltf_mesh() method, in most cases, they just want to view or use a specific Mesh from the GLTF file. Setting its local.matrix would be completely meaningless. Moreover, when not loading the entire node graph structure but directly setting the local.matrix. it can lead to some potential issues. The GLTF node stores the local transform matrix information, but the local transform is closely related to the node graph structure.

Although we can convert the local matrix to the world matrix by reconstructing the node graph, and assigning this calculated world matrix to the local matrix of the Mesh (which is likely what the current trimesh implementation does). It can also lead to potential problems when the user tries to compose these separately loaded Meshes into a node graph (through operations like mesh1.add(mesh2)).

@legut2
Copy link

legut2 commented Apr 16, 2024

I specifically want to load .gltf files and recently tried to load a model as a .gltf that doesn't have full support. For example, KHR_materials_transmission is missing as specified here. I'm glad to see full GLTF 2.0 support is being worked on!

Copy link
Collaborator

@almarklein almarklein left a comment

Choose a reason for hiding this comment

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

So IIUC the difference of load_mesh_gltf() compared to load_mesh() is that it also loads the transform?

The docs build is failing, but I don't think its related to this pr: WARNING: cannot cache unpickable configuration value: 'sphinx_gallery_conf'

@panxinmiao
Copy link
Contributor Author

So IIUC the difference of load_mesh_gltf() compared to load_mesh() is that it also loads the transform?

The new load_gltf_mesh() method does not load transforms, because I think this is meaningless and may also cause confusion.

@panxinmiao
Copy link
Contributor Author

Are there any other review comments on this PR? I need this PR to be merged in order to proceed with #715. 😄

Copy link
Collaborator

@Korijn Korijn left a comment

Choose a reason for hiding this comment

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

Let's go ahead and merge.

I do want to express my concern with regards to the public API: there are now four or five different load functions and you can pass a .gltf file to all of them I think. It's definitely confusing for users who are exploring the API and figuring out which one to use.

I'm thinking we can do better. I like Almar's earlier suggestion to rename the trimesh based load functions. We can also consider having a single load function with a backend keyword argument to select between trimesh and gltflib.

@Korijn Korijn merged commit 9ac332b into pygfx:main Apr 18, 2024
12 checks passed
@panxinmiao
Copy link
Contributor Author

I'm thinking we can do better. I like Almar's earlier suggestion to rename the trimesh based load functions. We can also consider having a single load function with a backend keyword argument to select between trimesh and gltflib.

The main issue is that the current behavior and return values of the load_gltf tool are somewhat different from the previous tool (via trimesh).

Additionally, I think the current load_gltf is still experimental and has not fully implemented all the functionality, so there may be other behavioral changes in the future. It might be better to wait until load_gltf is more mature before considering this.

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.

None yet

4 participants