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: ignore up direction for COLLADA #82

Closed
wants to merge 2 commits into from

Conversation

xqms
Copy link
Contributor

@xqms xqms commented Mar 16, 2020

Set the AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION flag to true.

If this flag is set to false (default), Assimp will return a transformation in the root aiNode to rotate the scene to Y-Up. Trade::AssimpImporter ignores this transform by default, since we only look at the children of the root node. But if the user sets the PreTransformVertices flag, Assimp will apply this transformation for us. Since nobody needs that, switch it off.

TLDR: Without this, things rotate when you set PreTransformVertices=true.

Otherwise we get ugly inconsistent behavior with the PreTransformVertices
flag.
@mosra mosra added this to the 2020.0a milestone Mar 22, 2020
@mosra
Copy link
Owner

mosra commented Mar 22, 2020

Running late, catching up with everything I missed, sorry :)

Interesting. Can you provide a tiny test case so this doesn't get accidentally broken/reversed in the future? Thanks! 🙏

@codecov-io
Copy link

Codecov Report

Merging #82 into master will decrease coverage by 0.39%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #82     +/-   ##
=========================================
- Coverage   92.21%   91.81%   -0.4%     
=========================================
  Files          76       76             
  Lines        4968     4631    -337     
=========================================
- Hits         4581     4252    -329     
+ Misses        387      379      -8
Impacted Files Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 85.22% <71.42%> (-2.88%) ⬇️
...agnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp 94.07% <0%> (-0.41%) ⬇️
src/Magnum/OpenDdl/Document.h 88.57% <0%> (-0.32%) ⬇️
...mPlugins/JpegImageConverter/JpegImageConverter.cpp 96.1% <0%> (-0.15%) ⬇️
...lugins/BasisImageConverter/BasisImageConverter.cpp 87.61% <0%> (-0.12%) ⬇️
src/Magnum/OpenDdl/OpenDdl.cpp 90.64% <0%> (-0.12%) ⬇️
src/MagnumPlugins/FreeTypeFont/FreeTypeFont.cpp 98.73% <0%> (-0.07%) ⬇️
...agnumPlugins/StanfordImporter/StanfordImporter.cpp 99.31% <0%> (-0.05%) ⬇️
src/MagnumPlugins/OpenGexImporter/openGexSpec.hpp 100% <0%> (ø) ⬆️
... and 16 more

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...7b24128. Read the comment docs.

@xqms
Copy link
Contributor Author

xqms commented Apr 1, 2020

Sorry for the long delay - here's a test case. I used the existing line.dae file, but had to add an <up_axis> tag to it to trigger Assimp's wonky correction logic. Let me know if you would rather have a separate dae file for this test.

This tag is always added by blender on export to Collada.

@mosra
Copy link
Owner

mosra commented Apr 4, 2020

Sorry for the long delay

No problem, I hope everything is okay (as much as current global situation allows) on your side :)

I'm just merging this, and realized that maybe a better fix for this would be not ignoring the root node and its Z_UP but instead incorporating it into the topmost object transformations? That way the transformed mesh positions would be always the same, with either:

  • transformation being identity and vertex data pretransformed,
  • or transformation correcting for Z-up and vertex data unchanged

What do you say? I did a quick implementation of this here and it seems like a better solution than
importing the file consistently but clearly oriented wrong.

@mosra mosra added this to TODO in Asset management via automation Apr 4, 2020
@mosra
Copy link
Owner

mosra commented Apr 4, 2020

I just pushed the above-described variant as de0c46e (into the next branch). Could you confirm it works well enough for you before I put it to master? Thanks a lot! 🙏

@mosra mosra moved this from TODO to In progress in Asset management Apr 4, 2020
@xqms
Copy link
Contributor Author

xqms commented Apr 4, 2020

Everything is fine here, thanks for asking. We just had to scramble to find remote teaching solutions for our student courses, which is more difficult for practical work. But I like challenges ;)

I didn't test your solution yet, I'll do so tomorrow.

But I have to say that I dislike these re-orientation heuristics, so I'd vote for disabling it. In case of the test file, the correction rotation isn't even well-defined - if Z is up, what is forward? Additionally, in many cases that are interesting for me, "up" doesn't mean anything (e.g. a robot hand). So generally I like software that doesn't mess with my data - X should stay X and Z should stay Z.

Now you say "then don't put an up_axis tag in your DAE", but everyone puts one in ;) Blender for example doesn't have the option to omit it.

Maybe we could offer a separate "up correction" transform in the importer interface? The user could then choose to apply it or ignore it...

@mosra
Copy link
Owner

mosra commented Apr 4, 2020

Now you say "then don't put an up_axis tag in your DAE", but everyone puts one in ;) Blender for example doesn't have the option to omit it.

The goal I had here was -- if I export a COLLADA using default Blender settings (Z up) and then I export glTF also using the default settings (Y up), an app (such as magnum-player) should display them the same way. I just exported two cubes out of blender as a *.dae and *.gltf and this commit made them open the same way, while that was broken before. So I think this is a step in the right direction. Of course for many formats (OBJ, STL) the up direction or handedness is undefined, so there you can only guess or hope (and the importer thus won't attempt touching the data in any way). But that's the same for images, you can't generally know if a PNG is sRGB or linear.

heuristics

It would be heuristics if it attempted to guess the up direction from metadata in the file ("if exported by 2.77.3 < blender < 2.78, attempt X up because it was broken there for a while"). I think this is defined pretty clearly considering how overengineered COLLADA is -- Y up and right-handed unless overriden :)

Maybe we could offer a separate "up correction" transform in the importer interface?

What about exposing AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION to the user?

@xqms
Copy link
Contributor Author

xqms commented Apr 5, 2020

I think this is defined pretty clearly considering how overengineered COLLADA is -- Y up and right-handed unless overriden :)

Ok, I had to look it up in the COLLADA reference. up_axis=Z_UP defines the other axes as well (right=+X and thus "in"=-Y). Seems arbitrary to me, but at least it's defined. I rest my case ;)

What about exposing AI_CONFIG_IMPORT_COLLADA_IGNORE_UP_DIRECTION to the user?

Would also work for me. Do you think it's worthwhile to define a configuration value with a generic name that could apply for all importers that have such mechanisms? E.g. group general, key IgnoreUpDirection, or CorrectOrientation?

@xqms
Copy link
Contributor Author

xqms commented Apr 5, 2020

Btw, the application I work on with a group of students is a VR replacement/twin of http://wiki.ros.org/rviz. Here, one task is to display the robot's current posture. All robot links are available as separate COLLADA files that have to be displayed at the correct 6D pose. If the importer now applies a Y-up "correction" on each individual mesh, the resulting poses will be incorrect.
Note that there is no "correct" correction to apply in this case (other than identity). If at all, such a correction has to be applied at the robot's base, once for all links. So we definitely need an option to switch it off (my students previously came up with a look-up table of file types to applied corrections in order to correct things).

rviz specifically ignores the transformation on the root aiNode, same as the Magnum importer did so far:
https://github.com/ros-visualization/rviz/blob/8a010bbda36d425129dc06887eddcab032459319/src/rviz/mesh_loader.cpp#L232

@mosra
Copy link
Owner

mosra commented Apr 5, 2020

Do you think it's worthwhile to define a configuration value with a generic name that could apply for all importers that have such mechanisms?

I need to do such a thing for image Y-flip for Vulkan, so this could go there as well ... and not via a stringly typed configuration, but rather a proper flag (importer->setFlags(...)), together with other things like verbose output, forcing zero-copy import and such. Until very recently this was all blocked by the monstrous MeshData rework. However I think this should be only exposed for importers / formats that actually do such a thing, which to my knowledge is only Assimp and OpenGEX (but nobody uses that anymore). OBJ, PLY, STL files have no up axis definition and glTF assumes Y up always -- I don't think it makes sense to introduce additional logic in those importers to perform reorientation, I'd rather see that done in the user code instead.

Anyway, until this is done I'll expose this COLLADA-specific option the "classic" way, via configuration(). To match the option name, does a group [config] (instead of [postprocess]) and ImportColladaIgnoreUpDirection sound okay?

Note that there is no "correct" correction to apply in this case

I guess you'd need to patch all COLLADA files to say Y up or nothing, and that's ... like fixing the world, impossible :D

@xqms
Copy link
Contributor Author

xqms commented Apr 5, 2020

Anyway, until this is done I'll expose this COLLADA-specific option the "classic" way, via configuration(). To match the option name, does a group [config] (instead of [postprocess]) and ImportColladaIgnoreUpDirection sound okay?

Yep, works for me :)

I'm pretty excited about your MeshData rework, by the way 👍 Especially serialization will be a huge time-saver in applications where I have to deal with third-party meshes. I have a crude caching system in place for simplified collision meshes, but importing the original visual assets every time (especially via Assimp) takes a lot of time.

Feel free to close the PR, your implemented solution already fixes the problem, and getting the configuration flag will make everything work for me :)

@mosra
Copy link
Owner

mosra commented Apr 6, 2020

Okay, this should be now completed in 0177802 and 7c08775. Thanks for keeping Assimp in shape :)

FYI, there's also openState() where you can initialize & configure assimp in whichever way you need and then let Magnum do the boring data extraction work. It's used inside DartIntegration this way, but not much known otherwise. This could be useful in hypothetical future cases where you'd again need to enable/disable something the plugin doesn't do on its own.

@mosra mosra closed this Apr 6, 2020
Asset management automation moved this from In progress to Done Apr 6, 2020
@xqms xqms deleted the feature/assimp_pretransform branch April 7, 2020 11:24
@xqms
Copy link
Contributor Author

xqms commented Apr 7, 2020

FYI, there's also openState()

I hadn't noticed that one. Thanks!

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