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

Expose model_unknown_space parameter to ROS2 #448

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DPR00
Copy link
Collaborator

@DPR00 DPR00 commented Oct 22, 2024

Proposed changes

Expose model_unknown_space parameter of the Likelihood Field sensor model to Beluga AMCL (ROS 2).

Type of change

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

💥 Breaking change! Explain why a non-backwards compatible change is necessary or remove this line entirely if not applicable.

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

Anything worth mentioning to the reviewers.

@DPR00 DPR00 requested a review from hidmic October 22, 2024 01:06
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.

Looks about correct. Have we tested it in simulation or against some dataset already?

@@ -120,6 +120,12 @@ AmclNode::AmclNode(const rclcpp::NodeOptions& options) : BaseAMCLNode{"amcl", ""
declare_parameter("z_rand", rclcpp::ParameterValue(0.5), descriptor);
}

{
auto descriptor = rcl_interfaces::msg::ParameterDescriptor();
descriptor.description = "If false, Likelihood Field sensor model won't model unknown space.";
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DPR00 consider having matching descriptions in ROS 1 and ROS 2. I like the ROS 1 one better.

@@ -61,6 +61,8 @@ amcl:
z_hit: 0.5
# Weight used to combine the probability of random noise in perception.
z_rand: 0.5
# Whether to model unknown space or assume it free.
model_unknown_space : false
Copy link
Collaborator

Choose a reason for hiding this comment

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

@DPR00 nit:

Suggested change
model_unknown_space : false
model_unknown_space: false

Signed-off-by: Diego Palma <[email protected]>
@DPR00 DPR00 force-pushed the davpr/ros2_model_unknown_spaces branch from 1984aeb to 1469782 Compare November 8, 2024 19:22
@DPR00 DPR00 force-pushed the davpr/ros2_model_unknown_spaces branch from bac6245 to 9e6e7f8 Compare November 10, 2024 18:24
@hidmic hidmic self-requested a review November 12, 2024 14:16
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