-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
poweifeng
commented
Feb 9, 2024
•
edited
Loading
edited
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. |
Please take a look. If the approach looks ok, I'll add the java/js bindings. |
3a23947
to
e9c027a
Compare
- 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
e9c027a
to
872d243
Compare
struct AssetConfigurationExtended { | ||
//! Optional The same parameter as provided to \struct ResourceConfiguration ResourceLoader.h | ||
//! gltfio/ResourceLoader.h | ||
char const* gltfPath; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I'm breaking this into several changes, so marking as draft |
implemented in:
|