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

Add amcl3 demo #9

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add amcl3 demo #9

wants to merge 13 commits into from

Conversation

pvela2017
Copy link

Proposed changes

Added a demo for the AMCL3 algorithm based on the likelihood sensor model 3D (Ekumen-OS/beluga#440)

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

Checklist

Put an x in the boxes that apply. This is simply a reminder of what we will require before merging your code.

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All commits have been signed for DCO

Additional comments

Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
@hidmic hidmic self-requested a review November 25, 2024 11:30
Copy link
Collaborator

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

First pass

repositories:
beluga:
type: git
url: https://github.com/pvela2017/beluga.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 this needs to change before merging.

libopenvdb-dev \
python3-pip \
mc \
ros-jazzy-pcl-ros \
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 why adding this here instead of in the corresponding package.xml?

Copy link
Author

Choose a reason for hiding this comment

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

Changed in 150be6e

]

return LaunchDescription(nodes)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 nit: missing EOF newline, here and elsewhere.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in b15a8cc and 471d716

@@ -0,0 +1,120 @@
#!/usr/bin/env python3

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 hmm, why do we need to publish markers? Why not just use RViz displays for poses?

Copy link
Author

Choose a reason for hiding this comment

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

I like the line type visualization

@@ -0,0 +1,63 @@
#!/usr/bin/env python3

Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 meta: I wonder if it wouldn't be simpler to modify the dataset (which we have already modified) to include odom to base_link transforms.

Copy link
Author

Choose a reason for hiding this comment

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

I will try


// Initial position
const auto pose = Sophus::SE3d{
Sophus::SO3d{/*kInitialPoseYaw, kInitialPosePitch, kInitialPoseRoll*/},
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 nit: remove commented out code.

Copy link
Author

Choose a reason for hiding this comment

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

Changed in 64e5615

point_color.b = 1.0;
point_color.a = 1.0;

for (openvdb::FloatGrid::ValueOnCIter iter = grid->cbeginValueOn(); iter; ++iter) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 the condition expression is unusual, is there no cendValueOn()?

Copy link
Author

Choose a reason for hiding this comment

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

As discussed in the meeting it is ok.

OpenVDB iterators are not STL-compatible in that one can always request an iterator that points to the beginning of a collection of elements (nodes, voxels, etc.), but one usually cannot request an iterator that points to the end of the collection. (This is because finding the end might require a full tree traversal.)

Reference

visualization_msgs::msg::Marker marker_msg;
std_msgs::msg::ColorRGBA point_color;
openvdb::CoordBBox bbox = grid->evalActiveVoxelBoundingBox();
double min_z, max_z;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 nit: better define and assign on the same line, then it can be const

Copy link
Author

Choose a reason for hiding this comment

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

Changed in 64e5615

marker_msg.colors.push_back(point_color);
}

double size = grid->transform().voxelSize()[0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 nit: can be const

Copy link
Author

Choose a reason for hiding this comment

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

Changed in 64e5615

"localization/beluga_demo_bearing_localization"
"localization/beluga_demo_fiducial_localization"
"localization/beluga_demo_lidar_localization"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pvela2017 I'm a bit ambivalent about this, but I can't think of a better solution. CC @glpuga for thoughts.

Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Signed-off-by: Pablo Vela <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants