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

gltfio: use tangent space mesh #7566

Closed
wants to merge 2 commits into from
Closed

Conversation

poweifeng
Copy link
Contributor

@poweifeng poweifeng commented Feb 9, 2024

 - Fix missing attributes in TangentSpaceMesh
 - Fix missing reference in MikktspaceImpl.cpp
 - Add gltfio/src/extended to implement an alternate loader for
   primitives. This is largely based on the implementation in
   AssetLoader/ResourceLoader
 - Pull common methods between the two implementations into a
   utility namespace.
 - Switch gltf_viewer to use new gltf loader (extended loader).
 - Able to correctly produce flat shading from gltf that only have
   vertex positions and indices.
 - Able to run mikktspace to generate tangent spaces as per gltf
   spec
 - Note that java/webgl samples have not been ported to this
   framework due to difficulty in maintaining interaction between
   AssetLoader/ResourceLoader

Fixes #7444; Fixes #6358

@poweifeng poweifeng requested review from romainguy, pixelflinger and z3moon and removed request for romainguy and pixelflinger February 9, 2024 18:46
Copy link

github-actions bot commented Feb 9, 2024

Please add a release note line to NEW_RELEASE_NOTES.md. If this PR does not warrant a release note, add the 'internal' label to this PR.

@poweifeng poweifeng added the internal Issue/PR does not affect clients label Feb 9, 2024
@poweifeng
Copy link
Contributor Author

Please take a look. If the approach looks ok, I'll add the java/js bindings.

@poweifeng poweifeng force-pushed the pf/gltfio-tangent-space-mesh branch 3 times, most recently from 3a23947 to e9c027a Compare February 13, 2024 19:42
 - Fix missing attributes in TangentSpaceMesh
 - Fix missing reference in MikktspaceImpl.cpp
 - Add gltfio/src/extended to implement an alternate loader for
   primitives. This is largely based on the implementation in
   AssetLoader/ResourceLoader
 - Pull common methods between the two implementations into a
   utility namespace.
 - Switch gltf_viewer to use new gltf loader (extended loader).
 - Able to correctly produce flat shading from gltf that only have
   vertex positions and indices.
 - Able to run mikktspace to generate tangent spaces as per gltf
   spec
 - Note that java/webgl samples have not been ported to this
   framework due to difficulty in maintaining interaction between
   AssetLoader/ResourceLoader

Fixes #7444; Fixes #6358
@poweifeng poweifeng changed the title [WIP] gltfio: use tangent space mesh gltfio: use tangent space mesh Feb 22, 2024
@poweifeng poweifeng marked this pull request as ready for review February 22, 2024 22:45
struct AssetConfigurationExtended {
//! Optional The same parameter as provided to \struct ResourceConfiguration ResourceLoader.h
//! gltfio/ResourceLoader.h
char const* gltfPath;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a significant philosophical shift in API. With this change, now the AssetLoader is locked to a specific glTF path. Before, this was limited to ResourceLoader. I don't have a good solution off the top of my head, but I would be in favor of removing the concept of the "file system" from gltfio. Instead, we could require the client to search the file system and provide all the resource data through addResourceData. In fact, this is what iOS, Android, and Web already do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interesting point. I didn't really understand the need to separate AssetLoader and ResourceLoader, and your explanation makes that much clearer.

I think, longterm, if I'm able to, I'd like to move toward the future you described where filesystem is abstracted out. And then we wouldn't have to have two API entrance points for loading gltf. For now, this is kind of just hacking to make things work on top of the existing two classes.

Still, I'd like to introduce this change, which will admittedly only benefit desktops and only when all the data are on disk, but it'll definitely make the desktop gtlf_viewer nicer.

Choose a reason for hiding this comment

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

I'd like to add on to this that the ability to load data from anywhere, not just the filesystem, is really quite important, whether it's loading from a network, or a compressed archive of some kind. Only being able to load from regular filesystem files isn't flexible enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I'll likely add to this with a more general loading mechanism in a follow-up. Thank you for the comment.

@poweifeng poweifeng marked this pull request as draft March 13, 2024 00:11
@poweifeng
Copy link
Contributor Author

I'm breaking this into several changes, so marking as draft

@poweifeng
Copy link
Contributor Author

poweifeng commented Apr 29, 2024

implemented in:

@poweifeng poweifeng closed this Apr 29, 2024
@poweifeng poweifeng deleted the pf/gltfio-tangent-space-mesh branch April 29, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants