fix(autoware_ndt_scan_matcher): zero div if particles_num < 20#996
fix(autoware_ndt_scan_matcher): zero div if particles_num < 20#996kazu-321 wants to merge 7 commits intoautowarefoundation:mainfrom
Conversation
Signed-off-by: kazu-321 <kzs321kzs@gmail.com>
|
Thank you for contributing to the Autoware project! 🚧 If your pull request is in progress, switch it to draft mode. Please ensure:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a runtime crash in autoware_ndt_scan_matcher that can occur during Monte Carlo initial pose estimation when initial_pose_estimation.particles_num is less than 20, by preventing a modulo-by-zero in the marker publish logic.
Changes:
- Guarded the marker publish condition to avoid evaluating
(i + 1) % publish_intervalwhenpublish_interval == 0. - Minor formatting adjustment to the
out_of_map_rangewarningifstatement.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: kazu-321 <kzs321kzs@gmail.com>
287b0ab to
003e245
Compare
| (publish_interval != 0 && (i + 1) % publish_interval == 0) || | ||
| (i + 1) == param_.initial_pose_estimation.particles_num) { |
There was a problem hiding this comment.
Consider fixing this at the source by clamping publish_interval to at least 1, rather than guarding the modulo downstream.
Change line 1115
to:
const int64_t publish_interval =
std::max<int64_t>(param_.initial_pose_estimation.particles_num / publish_num, 1);Then this condition can stay unchanged:
| (publish_interval != 0 && (i + 1) % publish_interval == 0) || | |
| (i + 1) == param_.initial_pose_estimation.particles_num) { | |
| (i + 1) % publish_interval == 0 || (i + 1) == param_.initial_pose_estimation.particles_num) { |
This eliminates the zero divisor at the point of definition instead of working around it at the point of use. For small particle counts (< 20), it publishes progress every iteration, which is fine since the total is still under the 20-publish budget.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
=======================================
Coverage 55.97% 55.98%
=======================================
Files 378 378
Lines 23654 23654
Branches 11175 11176 +1
=======================================
+ Hits 13241 13242 +1
Misses 7540 7540
+ Partials 2873 2872 -1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: kazu-321 <kzs321kzs@gmail.com>
…are_core into fix/ndt_particle_zero_div
|
@kazu-321 This looks fine but do you have any dataset where the error occurs? I want to confirm that the patch can really fix the issue, i.e. before applying the patch the error occurred, but after applying the patch the error has gone. |
Description
This fixes an error that occurs when
particles_numis less than 20.publish_intervalbecomes 0, resulting in division by zero.This causes the node to crash with an error at runtime.
This change avoids division by zero by adding
publish_interval != 0before the division ifpublish_intervalis 0.Related links
Parent Issue:
How was this PR tested?
Confirm that no error occurs when particles_num is set to less than 20.
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.