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

Add Assimp support. #3910

Closed
wants to merge 3 commits into from
Closed

Add Assimp support. #3910

wants to merge 3 commits into from

Conversation

tanukibouwer
Copy link
Contributor

@tanukibouwer tanukibouwer commented Nov 10, 2021

This PR adds an AssimpParser to Trenchbroom that will load any entity model in a format that Assimp supports if another parser didn't get to import it first.
assimp is required as supported model format in the GameConfig.
This does not include animation, so all meshes are static.
Assimp supports mdl, so that means this includes Half-Life model support.
md5mesh and LWO are also included, but are untested.
Solves #3449.
Helps out #1140.
Solves #719, hopefully.
This code's pretty big and complicated compared to what I usually write so apologies if it's messy.
A fix for #3906 would pair well with this PR as importing 'modern' models will still take a while to do due to building the spatial tree.
image

@kduske
Copy link
Collaborator

kduske commented Nov 11, 2021

Thanks, this looks promising! It currently doesn't build and it needs a bit of work to get it into a mergeable state (style changes and such). Are you up for making such changes yourself? In that case I'd do a proper review and leave comments in the PR. I can also take over and redo the PR myself if you don't to address lots of comments.

Oh, and, this depends on #3883, correct?

@tanukibouwer
Copy link
Contributor Author

Thanks, this looks promising! It currently doesn't build and it needs a bit of work to get it into a mergeable state (style changes and such). Are you up for making such changes yourself? In that case I'd do a proper review and leave comments in the PR. I can also take over and redo the PR myself if you don't to address lots of comments.

Yeah, I'll be looking at this today and see if I can get this fixed.

Oh, and, this depends on #3883, correct?

Nope, my mistake. I only just noticed that commit was accidentally included.
This is my third time ever making a PR so I'm struggling a lot on the Git side, but I'll try to get it right!

@kduske
Copy link
Collaborator

kduske commented Nov 11, 2021

No worries, git is hard. You're doing a great job and I can help sort out any git history issues before we merge.

@RobertBeckebans
Copy link

Nice! 👍

@tanukibouwer
Copy link
Contributor Author

tanukibouwer commented Nov 11, 2021

Alright, I think I've fixed the compile issue.

No worries, git is hard. You're doing a great job and I can help sort out any git history issues before we merge.

Thanks, that'd be great.

@HeadClot
Copy link

HeadClot commented Nov 11, 2021

Bit of a question would this address #2628 and #2103 ? Even it it addresses it for future work I would be happy. Either way good work!

@tanukibouwer
Copy link
Contributor Author

tanukibouwer commented Nov 11, 2021

Bit of a question would this address #2628 and #2103 ? Even it it addresses it for future work I would be happy. Either way good work!

#2628 gets solved by this.
#2103 does not get solved by this, but Assimp does have the ability to export to glTF, so in the future we could create a general Assimp exporter that can export to any of Assimp's formats, making use of #3883's addition of export options.
And thanks!

@HeadClot
Copy link

Bit of a question would this address #2628 and #2103 ? Even it it addresses it for future work I would be happy. Either way good work!

#2628 gets solved by this. #2103 does not get solved by this, but Assimp does have the ability to export to glTF, so in the future we could create a general Assimp exporter that can export to any of Assimp's formats, making use of #3883's addition of export options. And thanks!

Thank you for the insight into your PR. Looking forward to seeing it merged into main :)

@WatIsDeze
Copy link

If in the meantime, IQM support could be added, we're golden. Because for some reason Assimp is just not into it.

Then again, for our project we might post our own PR soon enough if you guys are too occupied. No worries.

https://github.com/culacant/raylib/blob/1604fa9e475cbfaa1a1fede9d147435548bdf5ef/src/iqml.c

@kduske
Copy link
Collaborator

kduske commented Nov 11, 2021

If in the meantime, IQM support could be added, we're golden. Because for some reason Assimp is just not into it.

Then again, for our project we might post our own PR soon enough if you guys are too occupied. No worries.

https://github.com/culacant/raylib/blob/1604fa9e475cbfaa1a1fede9d147435548bdf5ef/src/iqml.c

I'm not sure what the relevance to this PR is?

@HeadClot
Copy link

If in the meantime, IQM support could be added, we're golden. Because for some reason Assimp is just not into it.

Then again, for our project we might post our own PR soon enough if you guys are too occupied. No worries.

https://github.com/culacant/raylib/blob/1604fa9e475cbfaa1a1fede9d147435548bdf5ef/src/iqml.c

My advice would be to make this a separate PR to add .IQM support. My 2 cents though.

@tanukibouwer
Copy link
Contributor Author

Compile tests should be working now.
Windows test was failing due to me using a minus operator where I shouldn't.
The Linux build was failing due to issues in Assimp's unit tests but those should now be disabled.

@WatIsDeze
Copy link

If in the meantime, IQM support could be added, we're golden. Because for some reason Assimp is just not into it.
Then again, for our project we might post our own PR soon enough if you guys are too occupied. No worries.
https://github.com/culacant/raylib/blob/1604fa9e475cbfaa1a1fede9d147435548bdf5ef/src/iqml.c

My advice would be to make this a separate PR to add .IQM support. My 2 cents though.

We will. We've already forked TB to do what we got to, of course our schedule is big too but it won't be too long from now. Then it at last has .iqm support :P

@tanukibouwer
Copy link
Contributor Author

@kduske The compile tests should work now, could you allow them to run, please?

@tanukibouwer
Copy link
Contributor Author

For some reason the Windows compile seems to be failing to link Assimp. I haven't been able to figure out why from the logs, so I'm gonna try compiling it on Windows myself, but installing QT is going to take ages so this might take a long while.

@ericwa
Copy link
Collaborator

ericwa commented Nov 13, 2021

Found the link error on Windows: https://stackoverflow.com/questions/64086365/unresolved-external-when-just-adding-an-incomplete-type-in-class-in-header

Changing the following in AssimpParser.h fixes it:

-class aiNode;
-class aiScene;
-class aiMesh;
+struct aiNode;
+struct aiScene;
+struct aiMesh;

@tanukibouwer
Copy link
Contributor Author

Found the link error on Windows: https://stackoverflow.com/questions/64086365/unresolved-external-when-just-adding-an-incomplete-type-in-class-in-header

Changing the following in AssimpParser.h fixes it:

-class aiNode;
-class aiScene;
-class aiMesh;
+struct aiNode;
+struct aiScene;
+struct aiMesh;

Thank you so much for the info, I still haven't managed to get TB to compile on Windows so I haven't even been able to try anything myself.
We now no longer seem to get any linker issues, but now we're getting an error from building the manual..?

...
  Generating gen-manual
  Generating gen-manual/images
  Building Custom Rule D:/a/TrenchBroom/TrenchBroom/app/CMakeLists.txt
  Generating gen-manual/_reset.css, gen-manual/_responsive.css, gen-manual/_specific.css, gen-manual/_variables.css, gen-manual/default.css, gen-manual/shortcuts_helper.js
  Generating gen-manual/index.html
  fatal: No names found, cannot describe anything.
  -- Using version description "unstable-refs_pull_3910_merge-6ab6b1f792aeac002ea412ffb7a33e5eba6a2c94" from environment variables TB_PULL_REQUEST_HEAD_SHA and GITHUB_REF
  Generating gen-manual/shortcuts.js
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(241,5): error MSB8066: Custom build for 'D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\a502fc3a8f4d61d0f9672e1a591f155d\gen-manual.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\8635effa45ca0cfd450b9c2810790814\_reset.css.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\8635effa45ca0cfd450b9c2810790814\images.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\684b38fc4b6a1527dc5058b0de531cc8\AxisRestriction.png.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\8635effa45ca0cfd450b9c2810790814\index.html.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\8635effa45ca0cfd450b9c2810790814\shortcuts.js.rule;D:\a\TrenchBroom\TrenchBroom\cmakebuild\CMakeFiles\2e180fc0465b9396928512ecc0196cf6\GenerateManual.rule;D:\a\TrenchBroom\TrenchBroom\app\CMakeLists.txt' exited with code -1073741515. [D:\a\TrenchBroom\TrenchBroom\cmakebuild\app\GenerateManual.vcxproj]
  Building Custom Rule D:/a/TrenchBroom/TrenchBroom/lib/assimp/tools/assimp_cmd/CMakeLists.txt
  CompareDump.cpp
  ImageExtractor.cpp
  Main.cpp
...

@kduske
Copy link
Collaborator

kduske commented Nov 13, 2021

Hey, don't worry about all of the conflicts -- I merged a branch that introduces clang-format. Once this branch is ready to merge, it's pretty easy to get it back to a mergeable state. Just let me know when you're ready.

@tanukibouwer
Copy link
Contributor Author

I've finally managed to figure out the problem. I had forgotten to get the Assimp DLL copied at build on Windows.
@kduske Feel free to review this now.

@ericwa
Copy link
Collaborator

ericwa commented Nov 14, 2021

Nice! I downloaded the Win64 build from CI, and it's displaying models from Half-Life for me (I'm using the Steam edition).

I haven't tested macOS or Linux yet.

I'd propose we statically link assimp on Linux. If we don't, our official .rpm/.deb installers are going to have to install our custom .so system wide, which sounds like a recipe for problems (it's also packaged in distros, e.g. https://packages.ubuntu.com/source/focal/assimp ). Alternatively, maybe we can tell the assimp cmake to use a suffix like assimp-tb for the .so?

(Also, I think we're going the right direction with building our own copy, as opposed to using distro provided copies - as the HL MDL support has only been stabilized in 5.1.0 which is only in prerelease and won't be in distros for a while: see: https://github.com/assimp/assimp/releases )

@tanukibouwer
Copy link
Contributor Author

I've now (hopefully!) fixed the axis problems, so models should now be displaying correctly.

Models were not displaying correctly.

Seems like I got some math and some assumptions about Assimp's workings wrong. In any case, it's all fixed now. In my own projects, everything in TB now lines up perfectly with the models in-game.
That one failing Ubuntu CI test now suddenly passes, no idea why. clang-format's still messed up, though.

In any case, this is ready for review again.

@jdolan-chwy
Copy link

This is exciting to see! And looks like it's quite close to being merged. Yay!

@fhomolka
Copy link
Contributor

I saw some IQM discussion over here, and I'd like to point out that Garrux added IQM support to Assimp, so any IQM requirements would also get fixed by this.

https://github.com/assimp/assimp/blob/3cca102531466f5e5f0af0d90858a23ad83ce662/code/AssetLib/IQM/IQMImporter.h

@codecat
Copy link

codecat commented Mar 13, 2022

Wanted to add another comment of support for merging this PR. 👍

Additionally, does this also successfully load textures from its .mtl files?

@tanukibouwer
Copy link
Contributor Author

Wanted to add another comment of support for merging this PR. +1

Additionally, does this also successfully load textures from its .mtl files?

Thanks! It does indeed read .mtl files. It currently gives a warning saying that there's a default material that it can't find a texture for that I have yet to look into but it doesn't have any effect on the resulting import.
image

@Nomad7
Copy link

Nomad7 commented Mar 23, 2022

Assimp supports mdl, so that means this includes Half-Life model support.

I'm looking at using TB for TFC mapping and would love to see this feature implemented :)

@RobertBeckebans
Copy link

Maybe you can use this: RobertBeckebans@7bf9e3c

@tanukibouwer tanukibouwer force-pushed the assimp branch 2 times, most recently from 24d6f10 to cbff98e Compare June 20, 2022 15:26
@tanukibouwer
Copy link
Contributor Author

I have updated the Assimp branch to be even with the master branch. Assimp is now also built with vcpkg, using the work of @RobertBeckebans. One of the MacOS checks is failing for some reason, but everything else seems to work fine.

@tanukibouwer
Copy link
Contributor Author

Once again, I have updated the Assimp branch to be up to date with 2022.1.

@kduske
Copy link
Collaborator

kduske commented Jun 26, 2022

I think I was finally able to fix the build problem on macOS. Could you rebase onto master one more time? Then it should become green.

@tanukibouwer
Copy link
Contributor Author

I think I was finally able to fix the build problem on macOS. Could you rebase onto master one more time? Then it should become green.

It seems the workflow 'release' routines aren't working. At least they all seem to build okay.

@kduske
Copy link
Collaborator

kduske commented Jun 27, 2022

Hmm, I don't understand this failure at all. It should only attempt to upload release builds when you are pushing a tag. Did you push any tags?

@tanukibouwer
Copy link
Contributor Author

Hmm, I don't understand this failure at all. It should only attempt to upload release builds when you are pushing a tag. Did you push any tags?

I did not, as far as I can tell.

@kduske kduske mentioned this pull request Jun 27, 2022
@kduske
Copy link
Collaborator

kduske commented Jun 27, 2022

Ok, I have no idea what's wrong with this PR, but it works in my own PR over here: #4048 -- so I will close this one and finalize the commits on the other PR. Hope that's okay with you?

@tanukibouwer
Copy link
Contributor Author

Ok, I have no idea what's wrong with this PR, but it works in my own PR over here: #4048 -- so I will close this one and finalize the commits on the other PR. Hope that's okay with you?

Sure, though it seems GH says the checks have passed now.

@kduske kduske closed this Jun 27, 2022
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

10 participants