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

Do not assume axis orientation with FBX imports #456

Closed
7 tasks done
carmenfan opened this issue Mar 26, 2021 · 4 comments · Fixed by #489
Closed
7 tasks done

Do not assume axis orientation with FBX imports #456

carmenfan opened this issue Mar 26, 2021 · 4 comments · Fixed by #489

Comments

@carmenfan
Copy link
Member

carmenfan commented Mar 26, 2021

Description

We currently assume FBX files comes in on user space coordinates (i.e Z up) Whilst this is mostly the case but it's not always.
Assimp doesn't auto transform the coordinates for us. but by the sound of this we can find out and adjust accordingly
NOTE: if we do this we need to make sure we undo it if assimp ever decide to do this automatically!

Tasks

  • Update build process to use the latest assimp (locally)
  • Establish current behaviour on import in the context of the coordinate systems involved
  • Find the API from assimp that can identify the up/forward axis (used blender export options to help)
  • Create the loop that imports fbx -> exports fbx/obj to test any flipping (assuming no flipping occurs on export)?
  • If the above step is too much of a pain in the ass then, just test in local 3drepo.io with the custom bouncer version)
  • Update the code to take the assimp detected up/forward axis
  • Test to confirm that import occurs as expected

Related tickets

@har00n-haider har00n-haider self-assigned this Aug 5, 2021
@har00n-haider
Copy link
Contributor

@carmenfan for the ticket #405, what did you have in mind? I thought it might be good to tackle that as part of this ticket.

@carmenfan
Copy link
Member Author

@har00n-haider i think #405 is obsolete with this if you add tests.

basiclaly add tests on the importer and check if the boolean gives you the expected value is good enough i think.

@carmenfan
Copy link
Member Author

@har00n-haider

Find the API from assimp that can identify the up/forward axis (used blender export options to help)

It should just be this assimp/assimp#849 (comment)

Aiscene is what we get when the file is imported. so we should have an instance of aiscene already:
image

@har00n-haider
Copy link
Contributor

har00n-haider commented Aug 5, 2021

@carmenfan this option might be DOA. I am running through this loop and getting only the UnitScaleFactor entry. Just running it with the random FBX files on my PC.

image

Result :

image

Weird thing is the test fbx file provided in that thread, has the properties in it (ASCII version):
image

Is there something special we need to do to pull the metadata?

har00n-haider pushed a commit that referenced this issue Aug 10, 2021
har00n-haider pushed a commit that referenced this issue Aug 10, 2021
har00n-haider pushed a commit that referenced this issue Aug 19, 2021
carmenfan added a commit that referenced this issue Aug 20, 2021
)

* ISSUE #456 ISSUE #456 adding a new test file for assimpt file import

* ISSUE #456 ISSUE #456 first draft of testing the transformation from assimp meta for fbx

* ISSUE #456 ISSUE #456 updating orientation function to behave correctly when no metadata. Comments.

* ISSUE #456 ISSUE #456 adding some tests for fbx import

* ISSUE #456 adding info message to confirm pass on travis (seems a little weird...)

* Update bouncer/src/repo/manipulator/modelconvertor/import/repo_model_import_assimp.cpp

re writing bool check

Co-authored-by: Carmen Fan <wind5320@hotmail.com>

* ISSUE #456 applying the orientation correction to the root transform, rather than overwriting it

Co-authored-by: Carmen Fan <wind5320@hotmail.com>
@har00n-haider har00n-haider removed their assignment Aug 23, 2021
@carmenfan carmenfan added this to the v4.19.0 milestone Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants