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

feat(hesai): implement mask-based pruning filter for Hesai #251

Merged
merged 12 commits into from
Feb 19, 2025

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Jan 16, 2025

PR Type

  • New Feature

Related Links

Description

This PR implements the mask-based pruning filter introduced in #250 for all Hesai sensors. For more specifics on sensor configuration changes and filter behavior, see #250.

Review Procedure

  • Test whether pointcloud output is as expected when not specifying a filter
  • Try and create a downsample mask, e.g. in GIMP, and see if it works expected

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

@mojomex
Copy link
Collaborator Author

mojomex commented Jan 16, 2025

🟢 Self-Evaluation

🟢 Performance

Take-aways:

  • Performance impact is near 0 or negative (as in, performance improves when using the filter)
    • A disabled filter, and a filter that keeps all points, perform as well as the baseline before this PR
    • A filter discarding 50 % of points improves mean decode time by 7.4 %
    • A filter discarding all points improves mean decode time by 30.3 %

The below performance measurements have been performed with a modified decoder that measures timings of the convert_returns function and accumulates them for one scan. The filter for invalid points (distance == 0) has been disabled, as have other distance checks, to keep the number of input points artificially high ("worst-case"). The decoded number of points per cloud was around 460.000 which is around 2/3 of the maximum possible output in high-res+dual-return mode for OT128.

Decode times for different filter settings

label ('total_time', 'count') ('total_time', 'mean') ('total_time', 'std') ('total_time', 'min') ('total_time', '50%') ('total_time', 'max')
baseline 942 10.3296 0.390481 9.06057 10.3843 12.149
disabled 941 10.1409 0.296578 9.07079 10.0777 10.852
discard_all 941 7.19801 0.175469 6.94406 7.22401 7.68576
discard_half 940 9.56162 0.22284 9.16713 9.4687 10.6639
keep_all 940 10.2512 0.278572 9.13494 10.172 10.8176

@mojomex mojomex self-assigned this Jan 16, 2025
@mojomex mojomex changed the title Feat/hesai/universal downsample feat(hesai): implement mask-based pruning filter for Hesai Jan 16, 2025
@mojomex mojomex marked this pull request as draft January 16, 2025 05:59
@mojomex mojomex force-pushed the feat/hesai/universal-downsample branch from 9cde2d2 to cb20367 Compare January 16, 2025 08:26
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 37.77778% with 28 lines in your changes missing coverage. Please review.

Project coverage is 29.28%. Comparing base (9e40e2e) to head (2e8929b).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
nebula_ros/src/hesai/hesai_ros_wrapper.cpp 28.00% 15 Missing and 3 partials ⚠️
...s/nebula_decoders_hesai/decoders/hesai_decoder.hpp 44.44% 3 Missing and 2 partials ⚠️
...ommon/include/nebula_common/hesai/hesai_common.hpp 33.33% 2 Missing ⚠️
nebula_ros/src/hesai/decoder_wrapper.cpp 50.00% 2 Missing ⚠️
...ecoders/src/nebula_decoders_hesai/hesai_driver.cpp 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
- Coverage   29.32%   29.28%   -0.04%     
==========================================
  Files         108      112       +4     
  Lines        9580     9733     +153     
  Branches     3177     3252      +75     
==========================================
+ Hits         2809     2850      +41     
- Misses       6265     6359      +94     
- Partials      506      524      +18     
Flag Coverage Δ
differential 29.28% <37.77%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mojomex mojomex requested review from knzo25 and drwnz January 17, 2025 06:06
@mojomex mojomex marked this pull request as ready for review January 17, 2025 06:06
@mojomex mojomex force-pushed the feat/hesai/universal-downsample branch 2 times, most recently from f15429d to 2931084 Compare February 17, 2025 05:47
@mojomex
Copy link
Collaborator Author

mojomex commented Feb 17, 2025

@drwnz Rebased onto the updated #250, added missing support for XT16, please check 🙏

@mojomex mojomex force-pushed the feat/hesai/universal-downsample branch from 6e47dbb to 5fdca2a Compare February 17, 2025 13:17
mojomex added a commit that referenced this pull request Feb 18, 2025
commit 6e47dbb
Author: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date:   Mon Feb 17 05:47:57 2025 +0000

    ci(pre-commit): autofix

commit 2931084
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:32:01 2025 +0900

    docs(point_filters): add compatibility table for hesai

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 07ee5bf
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:31:24 2025 +0900

    feat(hesai): add downsample mask filter to decoder

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 3349bdd
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:29:59 2025 +0900

    chore(hesai): add FoV and resolution information to sensor definitions

    Signed-off-by: Max SCHMELLER <[email protected]>

commit ed23f8c
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:29:17 2025 +0900

    chore(hesai): add optional point_filters parameter to schema

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 5400657
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:28:56 2025 +0900

    chore(hesai): add downsample_mask_path config parameter

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 20a5c44
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:27:07 2025 +0900

    chore(nebula_ros): modernize how file extensions are replaced for calibration files

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 4cdc44a
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:18:47 2025 +0900

    chore(hesai_decoders): remove rclcpp logging dependency

    Signed-off-by: Max SCHMELLER <[email protected]>

commit cec2c76
Author: Max SCHMELLER <[email protected]>
Date:   Mon Feb 17 14:32:42 2025 +0900

    chore(angles): introduce `AngleUnit`s, make angle ranges support `end < start`angle.

    Signed-off-by: Max SCHMELLER <[email protected]>

commit a85ea4b
Author: Max SCHMELLER <[email protected]>
Date:   Mon Feb 17 14:31:47 2025 +0900

    chore(downsample_mask): fix imports

    Signed-off-by: Max SCHMELLER <[email protected]>

commit e18ae38
Author: Max SCHMELLER <[email protected]>
Date:   Fri Feb 14 16:46:05 2025 +0900

    fix(downsample_mask): add input validation for constructor parameters

    Add input validation checks for azimuth peak resolution, azimuth range extent, and number of channels to prevent divide-by-zeros and other issues

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 3082e0d
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 15:51:49 2025 +0900

    fix(downsample_mask): make `excluded()` function resilient to rounding errors

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 5655e32
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 15:26:46 2025 +0900

    chore(downsample_mask): explicitly cast instead of implicit conversions

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 1cadf2a
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 15:06:10 2025 +0900

    chore(downsample_mask): remove `.` before the exported mask's suffix

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 8e52dd9
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 15:01:56 2025 +0900

    chore(cspell): add milli-degrees (`mdeg`) to dictionary

    Signed-off-by: Max SCHMELLER <[email protected]>

commit fb6045f
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:15:26 2025 +0900

    docs(downsample_filter): add downsample filter docs

    Signed-off-by: Max SCHMELLER <[email protected]>

commit a14a069
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:10:34 2025 +0900

    test(downsample_mask): add unit tests for dithering and filtering

    Signed-off-by: Max SCHMELLER <[email protected]>

commit e2aed9b
Author: Max SCHMELLER <[email protected]>
Date:   Thu Jan 16 12:08:57 2025 +0900

    chore(downsample_mask): make debug mask output optional, clean up code

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 72aafb3
Author: Max SCHMELLER <[email protected]>
Date:   Wed Jan 15 16:09:32 2025 +0900

    chore(downsample_mask): add dependencies to nebula_decoders/package.xml

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 3c474d7
Author: Max SCHMELLER <[email protected]>
Date:   Wed Jan 15 16:04:19 2025 +0900

    chore(nebula_ros): add schema for point filters, and the downsample mask

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 219b789
Author: Max SCHMELLER <[email protected]>
Date:   Wed Jan 15 16:03:45 2025 +0900

    chore(downsample_mask): add downsample_mask filter class

    The filter takes a path to a PNG image (will be converted to grayscale) and expected resolution/channel count of the mask.

    The grayscale mask is then dithered into a black/white mask of the same dimensions, and an `excluded(NebulaPoint const&)` function is provided to test whether a point shall be excluded according to the mask.

    For debug purposes, the dithered mask is written to the same directory as the input mask, with the file ending changed to `_dithered.png`. If this fails, the filter will not throw but log a warning.

    Signed-off-by: Max SCHMELLER <[email protected]>

commit 65ed6ed
Author: Max SCHMELLER <[email protected]>
Date:   Wed Jan 15 16:03:37 2025 +0900

    chore(nebula_decoders): add utility types for angle pairs, ranges, and FoVs

    Signed-off-by: Max SCHMELLER <[email protected]>

Signed-off-by: Max SCHMELLER <[email protected]>
mojomex and others added 3 commits February 18, 2025 19:52
Before, an empty string was used to signal a disabled downsample filter but this was not very readable.

Signed-off-by: Max SCHMELLER <[email protected]>
@mojomex
Copy link
Collaborator Author

mojomex commented Feb 18, 2025

@drwnz I changed the std::string config item to std::optional<std::string> as discussed offline. I also added clearer instructions on which color settings to use to produce correct grayscale images.

Copy link
Collaborator

@drwnz drwnz left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

@mojomex mojomex merged commit 21d759b into tier4:main Feb 19, 2025
11 checks passed
@mojomex mojomex deleted the feat/hesai/universal-downsample branch February 19, 2025 09:50
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