Add static glTF support #14557
Add static glTF support #14557
Conversation
JosiahWI
force-pushed
the
feat/gltf-loader
branch
6 times, most recently
from
32c0180
to
9ad41e4
Compare
appgurueu
force-pushed
the
feat/gltf-loader
branch
from
9ad41e4
to
4ee428d
Compare
|
I have a branch based on this that also adds animation support now. You can just compile that branch to enjoy an animated gltf spider in Minetest. The only major thing left to be done is to actually run the tests there via catch2, which depends on #14565. |
|
I've tested @appgurueu's branch and it works fine! I think it deserves a proper PR, assuming this PR is stale. |
This PR is (like my branch, which is based on this) waiting on the Catch2 PR, so that we can get the unit tests to work. However, we don't know yet whether the other devs would like to review this as one big PR which adds supported for animated gltf, or first a slightly smaller PR to add support for static gltf, then a follow-up PR to add animation support. I personally would prefer one big PR because this lets me avoid rebase hell, but ultimately it's up to the reviewer to decide. |
|
This could already be reviewed in its current state. The work to get the unit tests to work with the build should not block any review efforts except that reviewers may have more trouble verifying that the tests work. We could also push a temporary patch so that the tests can be run separately from |
appgurueu
added
Roadmap
There was a problem hiding this comment.
As said, this has my approval as a co-author. There are still a few details in tiniergltf for me to sort out - nothing that negatively impacts loading of correct models, however - and the aforementioned To-Do of fixing the build to include unit tests. Still, this PR is ready to be reviewed.
appgurueu
added
One approval ✅ ◻️
Waiting (on dependency)
| #include "quaternion.h" | ||
| #include "vector3d.h" | ||
|
|
||
| #include "tiniergltf.hpp" |
There was a problem hiding this comment.
| #include "tiniergltf.hpp" | |
| #include <tiniergltf.hpp> |
There was a problem hiding this comment.
If we do this, I think we should treat it as one of the Irrlicht headers and not a vendored third-party lib.
| @@ -337,6 +340,8 @@ add_library(IRROBJ OBJECT | |||
| CMeshCache.cpp | |||
| ) | |||
|
|
|||
| target_link_libraries(IRROBJ PRIVATE IRRMESHOBJ) | |||
There was a problem hiding this comment.
IRROBJ needs the transitive include for the tiniergltf header.
There was a problem hiding this comment.
I think I have resolved this, but please check whether this is satisfactory.
|
One more question: since GLB avoids the need to handle base64 decoding and is more efficient, has supporting it been considered? |
|
Yes, it has been considered, and it should be quite an easy change (relative to animation for example), but I did not prioritize it for this PR because it works well as its own chunk of functionality that could be PRd separately. However, we could reconsider whether we want to initially support glTF or glb. |
appgurueu
force-pushed
the
feat/gltf-loader
branch
from
eea01c3
to
9027098
Compare
appgurueu
force-pushed
the
feat/gltf-loader
branch
from
10b4df4
to
572b311
Compare
JosiahWI
force-pushed
the
feat/gltf-loader
branch
from
742b0a3
to
b5c50d6
Compare
|
I have squashed the commits that were not fixes to recent review comments and rebased on master. |
This adds code for loading static glTF meshes to get a scaffold in place to keep the rest of the glTF feature PRs small and focused. There is some debate about whether that is worthwhile; I am marking this as draft. But every time I have to rebase this it takes up hours of my time, so I would like to land this very soon so it can be over with. This has been reviewed by me and @appgurueu.
To do
Ready for Review
How to test
The code is covered pretty thoroughly by Catch2 tests, which are waiting on #14656. The code is ready for review, though, and it should now work to load static glTF models. We have screenshots of this we could share as well.