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

AssimpImporter: support multiple meshes per Assimp node #81

Merged
merged 3 commits into from Mar 22, 2020

Conversation

xqms
Copy link
Contributor

@xqms xqms commented Mar 12, 2020

Since Magnum::MeshObjectData3D supports only one mesh instance, the importer has until now silently ignored any additional meshes per scene node.

We now add additional child objects for any extra meshes. This adds some ugliness to doObjectData3D() and friends, because we need to differentiate between "original" nodes referring to Assimp nodes and the new "additional mesh" objects.

This also adds a unit test using a hybrid quad/line mesh exported from blender.

@xqms xqms changed the title AssimpImporter: support multiple meshes per Assimp Node AssimpImporter: support multiple meshes per Assimp node Mar 12, 2020
@mosra
Copy link
Owner

mosra commented Mar 12, 2020

Hi, happy to see you around :)

Before I look at the code -- there was a variant of this submitted a while ago (in #68, without tests tho), is the handling in this PR different compared to that one? TinyGltfImporter implements this handling as well (see its docs), so maybe having the same behavior in both could make sense? (A potentially time-saving idea would be to reuse the same glTF test file for both and verifying both give the same result, but I have no idea how stable Assimp's glTF support is at this point.)

Thank you, as always!

@mosra mosra added this to TODO in Asset management via automation Mar 12, 2020
@xqms
Copy link
Contributor Author

xqms commented Mar 12, 2020

Thanks for the hint, I'll take a look at #68 and TinyGltfImporter :)

@mosra
Copy link
Owner

mosra commented Mar 12, 2020

Btw., I'm not saying the approach implemented there is the best possible -- if you see any shortcomings in it, I'm open for suggestions how to make it better :)

@xqms
Copy link
Contributor Author

xqms commented Mar 12, 2020

Well, at first glance it looks very similar to what I came up with, so of course it must be a great idea :D
The biggest difference seems to be the ordering of the objects and I'll adapt that to be consistent with TinyGltfImporter.

@xqms xqms force-pushed the feature/assimp_multi_mesh branch from 15e0b8f to 193ed34 Compare March 12, 2020 22:52
@codecov-io
Copy link

codecov-io commented Mar 12, 2020

Codecov Report

Merging #81 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   92.21%   92.23%   +0.02%     
==========================================
  Files          76       76              
  Lines        4968     4982      +14     
==========================================
+ Hits         4581     4595      +14     
  Misses        387      387
Impacted Files Coverage Δ
src/MagnumPlugins/AssimpImporter/AssimpImporter.h 100% <ø> (ø) ⬆️
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 88.5% <100%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67cbfa7...d1bc575. Read the comment docs.

@xqms
Copy link
Contributor Author

xqms commented Mar 12, 2020

Okay, I adapted the logic to be consistent with TinyGltfImporter and updated the documentation in AssimpImporter.h.

Unit test: Assimp (at least my stock version on Ubuntu 18.04, which is admittedly a bit old) won't load the glTF test file from TinyGltfImporter (just returns an empty scene, no meshes). I'll try with a more recent Assimp version tomorrow. Currently, we only have my more basic Collada test with quad/line primitives.

@xqms xqms force-pushed the feature/assimp_multi_mesh branch from 193ed34 to 47476b5 Compare March 12, 2020 23:54
xqms and others added 3 commits March 14, 2020 17:40
We basically introduce additional dummy objects just containing the
extra meshes as childs of the original objects (which still contain the
first mesh).
This tests the new support for multiple primitives and meshes per Assimp
node. Mostly ported from the corresponding TinyGltfImporter test case,
but using Collada format, since Assimp does not load the GLTF file properly.
@xqms xqms force-pushed the feature/assimp_multi_mesh branch from 47476b5 to d1bc575 Compare March 14, 2020 16:41
@xqms
Copy link
Contributor Author

xqms commented Mar 14, 2020

Updated again with a port of the TinyGltfImporter test case. I had to convert the example to COLLADA, because Assimp would just give me an empty scene from the gltf file.
I also had to add at least one triangle / line to each mesh, since Assimp strips out empty meshes.
Finally, the point primitive is not supported by COLLADA as far as I know, so I removed the point mesh.

I guess this is ready for review again, but there's no rush on my end ;)

@mosra mosra merged commit d1bc575 into mosra:master Mar 22, 2020
Asset management automation moved this from TODO to Done Mar 22, 2020
@mosra
Copy link
Owner

mosra commented Mar 22, 2020

Thank you!

The test was failing on Assimp 3.2 but I couldn't be bothered trying to find a variant that works there, so it's disabled on that ancient version (on the macOS CI runner it's 4.1 and there it's tested).

@mosra mosra added this to the 2020.06 milestone Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants