Add static glTF support #14557

JosiahWI · 2024-04-18T17:08:27Z

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. 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

Fix build
Include unit tests in build and run them. This depends on Upgrade client active object mgr tests to Catch2 #14565.
Make sure we keep only one copy of Catch2
Enable glTF in the Minetest engine portion of the codebase
Fix failing unit tests

How to test

The code is covered pretty thoroughly by Catch2 tests, which can be run with ./minetest --run-unittests --test-module "gltf".

Additionally the gltf devtest mod features some static glTF models that can be viewed in-game as nodes, entities, or in a formspec.

irr/include/IMesh.h

appgurueu · 2024-05-15T01:00:44Z

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.

Wuzzy2 · 2024-05-17T21:46:22Z

I've tested @appgurueu's branch and it works fine! I think it deserves a proper PR, assuming this PR is stale.

appgurueu · 2024-05-17T21:51:45Z

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.

JosiahWI · 2024-05-17T22:27:39Z

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 --run-unittests for review purposes. If a core dev is ready to review this, please go ahead.

appgurueu

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.

irr/include/IMesh.h

irr/include/SSkinMeshBuffer.h

irr/src/CGLTFMeshFileLoader.cpp

irr/include/SSkinMeshBuffer.h

irr/src/CGLTFMeshFileLoader.cpp

irr/src/CMakeLists.txt

irr/src/tests/CMakeLists.txt

sfan5 · 2024-05-21T16:47:51Z

One more question: since GLB avoids the need to handle base64 decoding and is more efficient, has supporting it been considered?

JosiahWI · 2024-05-21T16:54:26Z

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.

irr/src/CGLTFMeshFileLoader.h

irr/src/tests/CMakeLists.txt

This also corrects the function name to be snake_case because it is not a method.

- Alignment issues - Use os::Byteswap::byteswap - Directly write into matrices

Fix -Wsign-compare warning Fix snow man test texture coordinates The current texture coordinates specified in the test did not use sufficient precision. It is unfortunately common for software to round away the last bit when displaying floats. This issue was previously masked by Irrlicht using a float comparison with tolerance for vectors, which was changed in 3452857 to an exact float comparison for unrelated reasons. Manual inspection of the buffer reveals that the values produced by the reader are correct. Clean up unit tests Move everything to sections of one big "gltf" test case Fix triangle with vertex stride test case Improve docs Add copy & move constructor from STL string to Irrlicht string Move test assets to devtest, add corresponding entities Test fixes (get rid of device creation, fix error case tests) Address minor TODOs in tiniergltf Add spider model Work around lack of SKIP Implement clang-tidy suggestions in tiniergltf Update devtest mod count, add mod.conf Use SKIP now that we can Add applicable tests from animated branch Fix sparse accessor handling in tiniergltf Fix invalid bufferview bounds test Rename (& minify) files to follow naming conventions Improve logging Add frog gltf test model rebased minuscule refactor trailing whitespace let primitives share textures 🐸 fix gltf nodes fix model[] element add model[] test more trailing whitespace Update doc/lua_api.md Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com> { remove obsolete file explain use of terms in enums don't unnecessarily zero-initialize begone, base64! Improve documentation

Desour

Just took a small look, no full review.

A bit sad that glb is not yet supported.

Why was it again necessary and/or useful to create a new gltf lib? (Haven't found an answer in the discussions.)

irr/include/irrArray.h

irr/include/SSkinMeshBuffer.h

irr/include/irrString.h

src/unittest/test_irr_gltf_mesh_loader.cpp

doc/lua_api.md

appgurueu · 2024-08-29T12:17:27Z

A bit sad that glb is not yet supported.

I plan to add glb support in a subsequent PR. It is relatively easy to do. It doesn't have to be in this PR.

Why was it again necessary and/or useful to create a new gltf lib? (Haven't found an answer in the discussions.)

Because tinygltf was awful, especially for our purposes. The main reasons are:

tinygltf wasn't written with security in mind; it is not fit for reading untrusted media files. You can easily verify this by supplying some out of bounds indices for example (we have a unit test for this). There were some half-hearted attempts to add checks later but it's still an unsafe mess. Initially I tried to audit / patch tinygltf, but ultimately I found that writing a better alternative for just what we need to be much more feasible.
tinygltf introduces plenty of bloat - a gltf writer, support for two JSON libraries (none of which we use), support for embedded image loading (via another image library, though that can be turned off), etc. tinygltf.hpp alone has around 9kloc; the JSON library it uses by default (json.hpp) has 26kloc. In contrast, tiniergltf.hpp only adds 1.5kloc.
tinygltf code quality is, in my opinion, bad. I don't want to depend on this code.

Co-authored-by: Lars Mueller <appgurulars@gmx.de> Co-authored-by: jordan4ibanez <jordan4ibanez@users.noreply.github.com> Co-authored-by: sfan5 <sfan5@live.de> Co-authored-by: SmallJoker <SmallJoker@users.noreply.github.com>

JosiahWI mentioned this pull request Apr 18, 2024

Static glTF mesh loading minetest/irrlicht#200

Closed

6 tasks

JosiahWI force-pushed the feat/gltf-loader branch 6 times, most recently from 32c0180 to 9ad41e4 Compare April 18, 2024 18:56

appgurueu force-pushed the feat/gltf-loader branch from 9ad41e4 to 4ee428d Compare May 14, 2024 13:53

appgurueu reviewed May 14, 2024

View reviewed changes

irr/include/IMesh.h Outdated Show resolved Hide resolved

appgurueu mentioned this pull request May 14, 2024

glTF model support #14234

Closed

8 tasks

appgurueu added Roadmap The change matches an item on the current roadmap Feature ✨ PRs that add or enhance a feature @ Client rendering labels May 21, 2024

appgurueu approved these changes May 21, 2024

View reviewed changes

appgurueu added One approval ✅ ◻️ Waiting (on dependency) Waiting on another PR or external circumstances (not for rebases/changes requested) labels May 21, 2024

appgurueu marked this pull request as ready for review May 21, 2024 16:07

sfan5 requested changes May 21, 2024

View reviewed changes

JosiahWI commented May 21, 2024

View reviewed changes

irr/src/CGLTFMeshFileLoader.h Outdated Show resolved Hide resolved

irr/src/tests/CMakeLists.txt Outdated Show resolved Hide resolved

appgurueu force-pushed the feat/gltf-loader branch from eea01c3 to 9027098 Compare May 21, 2024 18:47

appgurueu requested a review from sfan5 May 21, 2024 21:58

appgurueu force-pushed the feat/gltf-loader branch from 10b4df4 to 572b311 Compare May 21, 2024 22:33

appgurueu mentioned this pull request May 21, 2024

[no squash] Add animated glTF support #14685

Merged

JosiahWI force-pushed the feat/gltf-loader branch from 742b0a3 to b5c50d6 Compare May 21, 2024 23:41

JosiahWI and others added 17 commits August 26, 2024 16:20

Use __BIG_ENDIAN_

b4a513d

This also corrects the function name to be snake_case because it is not a method.

Do not stack-allocate SSkinMeshBuffer

7d1cb8d

Fix/improve reading from accessors

259082e

- Alignment issues - Use os::Byteswap::byteswap - Directly write into matrices

Get unit tests to build

e6938c9

Fix minetestserver build

7b7c11f

Fix improper unit test paths

8e68091

Fix improper unit tests paths again

0234492

Fix strict aliasing violations in glTF tests

91f1862

Add f postfix to floating point test values

081ddea

Clamp tile index to prevent segfaults

ad927d1

Strip embedded image from glTF frog

bff4637

Disable backface culling for some test models

85dfa50

Proper gltf cube texture

2088f08

Minify textures

3708e43

Patch snow man

502deae

Trailing newlines

ba54f56

appgurueu force-pushed the feat/gltf-loader branch from 7ca15e0 to ba54f56 Compare August 26, 2024 14:20

Fix CI failure

4c40642

appgurueu force-pushed the feat/gltf-loader branch from 5a407af to 4c40642 Compare August 26, 2024 14:41

Desour reviewed Aug 29, 2024

View reviewed changes

appgurueu added 2 commits August 29, 2024 13:28

doc

aed59bb

forward

06f603a

appgurueu force-pushed the feat/gltf-loader branch from 7a9b58b to 06f603a Compare August 29, 2024 11:45

appgurueu added 3 commits August 29, 2024 14:30

can't forward that one

dedce21

include

a1d4043

move

75acbad

appgurueu merged commit ac11a14 into luanti-org:master Sep 2, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add static glTF support #14557

Add static glTF support #14557

JosiahWI commented Apr 18, 2024 •

edited by appgurueu

Loading

appgurueu commented May 15, 2024

Wuzzy2 commented May 17, 2024

appgurueu commented May 17, 2024

JosiahWI commented May 17, 2024

appgurueu left a comment

sfan5 commented May 21, 2024 •

edited

Loading

JosiahWI commented May 21, 2024 •

edited

Loading

Desour left a comment

appgurueu commented Aug 29, 2024 •

edited

Loading

Add static glTF support #14557

Add static glTF support #14557

Conversation

JosiahWI commented Apr 18, 2024 • edited by appgurueu Loading

To do

How to test

appgurueu commented May 15, 2024

Wuzzy2 commented May 17, 2024

appgurueu commented May 17, 2024

JosiahWI commented May 17, 2024

appgurueu left a comment

Choose a reason for hiding this comment

sfan5 commented May 21, 2024 • edited Loading

JosiahWI commented May 21, 2024 • edited Loading

Desour left a comment

Choose a reason for hiding this comment

appgurueu commented Aug 29, 2024 • edited Loading

JosiahWI commented Apr 18, 2024 •

edited by appgurueu

Loading

sfan5 commented May 21, 2024 •

edited

Loading

JosiahWI commented May 21, 2024 •

edited

Loading

appgurueu commented Aug 29, 2024 •

edited

Loading