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

fix: json algebra reading #1081

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

asalzburger
Copy link
Contributor

Change the order of translation / rotation when reading transforms back from json.

This is necessary that the transforms are correctly re-created.

@asalzburger asalzburger added Bug Something isn't working Component - Core Affects the Core module labels Nov 22, 2021
@asalzburger asalzburger added this to the next milestone Nov 22, 2021
@asalzburger asalzburger changed the title Fix: json algebra reading fix: json algebra reading Nov 22, 2021
@codecov
Copy link

codecov bot commented Nov 22, 2021

Codecov Report

Merging #1081 (33039ac) into main (f65cf50) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1081   +/-   ##
=======================================
  Coverage   48.64%   48.64%           
=======================================
  Files         341      341           
  Lines       17489    17489           
  Branches     8256     8256           
=======================================
  Hits         8507     8507           
  Misses       3208     3208           
  Partials     5774     5774           

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 f65cf50...33039ac. Read the comment docs.

Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ! Is the reason for this that Eigen Affine transform apply the translation first followed by the rotation ?

@paulgessinger
Copy link
Member

I don't think Eigen Affine transforms to anything special here. I think this is related to the code changed here using prerotate and pretranslate rather than simple matrix multiplications.

@Corentin-Allaire
Copy link
Contributor

Corentin-Allaire commented Nov 22, 2021

I am not sure I follow your point here, A.pretranslate(B) is just BxA no ? Instead of AxB for translate.

@paulgessinger
Copy link
Member

Exactly. But Eigen does not apply translation and rotation separately I don't think.

@Corentin-Allaire
Copy link
Contributor

I probably didn't express myself properly. What I meant is that the Transform3 type we use is equal to Translation x Rotation x Scaling. So if you want to build it with translate and rotate, translate need to go first; but if you want to use pretranslate and prerotate, prerotate need to go first. I think we agree and at that point it is just miscommunication :)

@kodiakhq kodiakhq bot merged commit ba86a7e into acts-project:main Nov 22, 2021
@paulgessinger paulgessinger modified the milestones: next, v15.1.0 Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Bug Something isn't working Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants