Pose with covariance display #76
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/gsoc-2022-potential-project-ideas/1339/4 |
Thodoris1999
force-pushed
the
pose_with_covariance_display
branch
from
fe14391 to
d03e221
Compare
|
Can you help me with how to remove those changes from history? I haven't done this before and don't want to accidentally mess anything up. |
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Thodoris1999
force-pushed
the
pose_with_covariance_display
branch
from
d03e221 to
e7b5a07
Compare
|
@ahcorde I dropped the "Add fortress support" commit with rebase |
ahcorde
requested changes
Apr 21, 2022
There was a problem hiding this comment.
Thank you, for this contribution! Some notes:
- Use 100 characters per line
- Run linters
colcon testyou might find a lot of linters errors
- Run linters
- Use the same style in the variable names. Camelcase and _camelCase in arguments
- Use ign-math instead of Eigen
- Use
PIMPL - Implement methods in the cpp
Comment on lines
18
to
19
| #include <ignition/rendering.hh> | ||
| #include <ignition/math.hh> |
| /** | ||
| * @brief Visualizes the covariance of a pose (orientation+position) using 2D and 3D ellipses | ||
| * | ||
| * One 3D ellipsoid indicates position covariance and 3 2D ellipsoids indicate the uncertainty of orientation along rotation at each axis. |
There was a problem hiding this comment.
you should use lines of 100 characters
| */ | ||
| void updateVisual() | ||
| { | ||
| for (int i = 0; i < 3; ++i) { |
There was a problem hiding this comment.
why 3 here? Use a constant
Comment on lines
56
to
59
| float shaftLength = 1.0; | ||
| float shaftRadius = 0.05; | ||
| float headLength = 0.25; | ||
| float headRadius = 0.1; |
There was a problem hiding this comment.
Suggested change
| float shaftLength = 1.0; | |
| float shaftRadius = 0.05; | |
| float headLength = 0.25; | |
| float headRadius = 0.1; | |
| float shaftLength = 1.0f; | |
| float shaftRadius = 0.05f; | |
| float headLength = 0.25f; | |
| float headRadius = 0.1f; |
Comment on lines
79
to
80
| float length = 1.0; | ||
| float radius = 0.1; |
There was a problem hiding this comment.
Suggested change
| float length = 1.0; | |
| float radius = 0.1; | |
| float length = 1.0f; | |
| float radius = 0.1f; |
| void update() override; | ||
|
|
||
| private: | ||
| ignition::rendering::RenderEngine * engine; |
There was a problem hiding this comment.
Use PIMPL. Use IGN_UTILS_IMPL_PTR from #include <ignition/utils/ImplPtr.hh>
ign_rviz_plugins/res/Plugins.qrc
Outdated
| @@ -42,11 +42,15 @@ | |||
| <qresource prefix="PoseDisplay/"> | |||
| <file alias="PoseDisplay.qml">qml/PoseDisplay.qml</file> | |||
| </qresource> | |||
|
|
|||
| @@ -0,0 +1,713 @@ | |||
| /* | |||
| * Copyright (C) 2020 Open Source Robotics Foundation | |||
There was a problem hiding this comment.
Suggested change
| * Copyright (C) 2020 Open Source Robotics Foundation | |
| * Copyright (C) 2022 Open Source Robotics Foundation |
Comment on lines
20
to
23
| double deg2rad(double degrees) | ||
| { | ||
| return degrees * 3.14159265358979 / 180.0; | ||
| } |
There was a problem hiding this comment.
remove this and use IGN_RTOD and IGN_DTOR from ign-math. Angle class defines them
| } | ||
|
|
||
| // Local function to force the axis to be right handed for 3D. Taken from ecl_statistics | ||
| void makeRightHanded(Eigen::Matrix3d & eigenvectors, Eigen::Vector3d & eigenvalues) |
There was a problem hiding this comment.
can you do this math with ign-math ? I would perfer not to add Eigen as a dependency
If there is something not in the API we might consider to add it there
|
Thank you for the review! I will get to the changes soon. As for the math, I used Eigen to be on a safe side with performance. I use diagonalization, which is very optimized in Eigen with a LAPACK implementation that takes advantage of the symmetric covariance https://eigen.tuxfamily.org/dox-devel/classEigen_1_1SelfAdjointEigenSolver.html#a110f7f5ff19fcabe6a10c6d48d5c419c. |
|
@ahcorde Sorry to bother, but I am having some issues using PIMPL with |
ahcorde
requested changes
Apr 25, 2022
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: ahcorde <[email protected]> Co-authored-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Thodoris1999
force-pushed
the
pose_with_covariance_display
branch
from
68eddd2 to
e829ee7
Compare
|
@ahcorde Did the merge By the way, the error I sent above was due to |
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
…doris1999/ign-rviz into pose_with_covariance_display Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Thodoris1999
force-pushed
the
pose_with_covariance_display
branch
from
79f97eb to
65e4d6a
Compare
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
|
Hi, I came back after catching up to school work. I tried to address the linter warnings:
if (user_data_.position_frame == Frame::Local &&
fixed_orientation_visual_->HasChild(position_root_visual_)) {
this->root_visual_->AddChild(fixed_orientation_visual_->RemoveChild(position_root_visual_));
} else if (user_data_.position_frame == Frame::Fixed &&
root_visual_->HasChild(position_root_visual_)) {
fixed_orientation_visual_->AddChild(this->root_visual_->RemoveChild(position_root_visual_));
}
if (user_data_.orientation_frame == Frame::Local &&
fixed_orientation_visual_->HasChild(orientation_root_visual_)) {
this->root_visual_->AddChild(fixed_orientation_visual_->RemoveChild(orientation_root_visual_));
} else if (user_data_.orientation_frame == Frame::Fixed &&
root_visual_->HasChild(orientation_root_visual_)) {
fixed_orientation_visual_->AddChild(this->root_visual_->RemoveChild(orientation_root_visual_));
}
Finally, there seems to be a DCO error: |
…e_display Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Signed-off-by: Theodoros Tyrovouzis <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎉 New feature
PoseWithCovariance display, largely inspired by the original implementation in RViz
More info in this post https://community.gazebosim.org/t/gsoc-2022-potential-project-ideas/1339/4
Checklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.