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

DartIntegration: use EigenIntegration and small check for NaN values #58

Closed
wants to merge 5 commits into from

Conversation

costashatz
Copy link
Contributor

This PR adds the following to the DartIntegration:

  • uses EigenIntegration when needed
  • adds a small check for NaN values (so that Magnum asserts do not break the flow of the program)

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/DartIntegration/CMakeLists.txt Outdated Show resolved Hide resolved
src/Magnum/DartIntegration/Object.cpp Outdated Show resolved Hide resolved

/* Pass it to Magnum */
_transformation.resetTransformation()
.rotate(theta, u)
.rotate(angle, axis)
Copy link
Owner

Choose a reason for hiding this comment

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

Why go through the pain of creating a quaternion and then again converting that to a transformation matrix inside rotate()? Wouldn't just the following work as well?

_transformation.setTransformation(trans);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error: ‘class Magnum::SceneGraph::AbstractBasicTranslationRotation3D<float>’ has no member named ‘setTransformation’; did you mean ‘resetTransformation’?

Copy link
Owner

Choose a reason for hiding this comment

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

blargh, forgot about this 😅 ... okay then, keep it as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you should provide this function? What is the rationale of not providing it for AbstractFeature?

Copy link
Owner

Choose a reason for hiding this comment

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

Because some transformations support only translation, or only translation/rotation, and the full transformation matrix could be too general for these.

... but I should provide rotate() taking a quat, definitely, adding that to my TODOs.

@codecov-io
Copy link

codecov-io commented Nov 6, 2019

Codecov Report

Merging #58 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
- Coverage   74.77%   74.44%   -0.33%     
==========================================
  Files          21       21              
  Lines         880      900      +20     
==========================================
+ Hits          658      670      +12     
- Misses        222      230       +8
Impacted Files Coverage Δ
src/Magnum/DartIntegration/Object.cpp 0% <0%> (ø) ⬆️
src/Magnum/ImGuiIntegration/Context.hpp 87.5% <0%> (-12.5%) ⬇️
src/Magnum/GlmIntegration/GtxIntegration.h 100% <0%> (ø) ⬆️
src/Magnum/GlmIntegration/GtcIntegration.h 100% <0%> (ø) ⬆️
src/Magnum/EigenIntegration/Integration.h 100% <0%> (ø) ⬆️
src/Magnum/GlmIntegration/Integration.h 100% <0%> (ø) ⬆️
src/Magnum/EigenIntegration/GeometryIntegration.h 100% <0%> (ø) ⬆️
src/Magnum/ImGuiIntegration/Context.h 100% <0%> (ø) ⬆️
src/Magnum/ImGuiIntegration/Context.cpp 83.13% <0%> (+0.09%) ⬆️

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 88bd1a2...dc8e10c. Read the comment docs.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Happy! Let's wait until the CIs finish, then I merge.

src/Magnum/DartIntegration/Object.cpp Outdated Show resolved Hide resolved
@mosra
Copy link
Owner

mosra commented Nov 6, 2019

Merged as f55a616 and 95d46a3, thank you!

@mosra mosra closed this Nov 6, 2019
@mosra mosra added this to the 2019.1c milestone Nov 6, 2019
@mosra mosra added this to Done in Physics Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Physics
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants