refactor(behavior_velocity_no_stopping_area_module): cppcheck warnings

[veqcc]

Description

I have made several chages based on CppCheck warnings.
Some commits are irrelevant to CppCheck.

1. remove redundant if branch
   e0e49be

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:94:13: style: Condition '!collision_points.empty()' is always true [knownConditionTrueFalse]
        if (!collision_points.empty()) {
            ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:90:35: note: Assuming that condition 'collision_points.empty()' is not redundant
        if (collision_points.empty()) {
                                  ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:94:13: note: Condition '!collision_points.empty()' is always true
        if (!collision_points.empty()) {
            ^

2. remove unused variable
   021129a

3. remove redundant initialization
   0eb34eb

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:333:20: style: Redundant initialization for 'ego_area_end_idx'. The initialized value is overwritten before it is read. [redundantInitialization]
  ego_area_end_idx = ego_area_start_idx;
                   ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:309:27: note: ego_area_end_idx is initialized
  size_t ego_area_end_idx = ego_area_start_idx;
                          ^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:333:20: note: ego_area_end_idx is overwritten
  ego_area_end_idx = ego_area_start_idx;
                   ^

4. remove unused dist_from_area_sum variable
   9e76436

planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:338:26: style: Variable 'dist_from_area_sum' is assigned a value that is never used. [unreadVariable]
      dist_from_area_sum += tier4_autoware_utils::calcDistance2d(pp.at(i), pp.at(i - 1));
                         ^

Question

I have one question: there seems to be several unused functions in debug.cpp.
Is it intentional? If not, it looks it's better to delete them in terms of maintenance and readbility.

planning/behavior_velocity_no_stopping_area_module/src/debug.cpp:134:0: style: The function 'createDebugMarkerArray' is never used. [unusedFunction]
visualization_msgs::msg::MarkerArray NoStoppingAreaModule::createDebugMarkerArray()
^
planning/behavior_velocity_no_stopping_area_module/src/debug.cpp:165:0: style: The function 'createVirtualWalls' is never used. [unusedFunction]
motion_utils::VirtualWalls NoStoppingAreaModule::createVirtualWalls()
^
planning/behavior_velocity_no_stopping_area_module/src/manager.cpp:54:0: style: The function 'launchNewModules' is never used. [unusedFunction]
void NoStoppingAreaModuleManager::launchNewModules(
^
planning/behavior_velocity_no_stopping_area_module/src/manager.cpp:77:0: style: The function 'getModuleExpiredFunction' is never used. [unusedFunction]
NoStoppingAreaModuleManager::getModuleExpiredFunction(
^
planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp:108:0: style: The function 'modifyPathVelocity' is never used. [unusedFunction]
bool NoStoppingAreaModule::modifyPathVelocity(PathWithLaneId * path, StopReason * stop_reason)
^

Tests performed

No

Effects on system behavior

Interface changes

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

[shmpwk]

@veqcc
Thank you for your OSS contribution in holiday!
Regarding your question about unused functions in debug.cpp, I agree to delete those functions. But let me make sure to ask @kosuke55 -san who seems to be a heavy user of no stopping area as a goal planner developper.

Other fixes looks goooooood to me!

[kosuke55]

@veqcc

I have one question: there seems to be several unused functions in debug.cpp.

thanks! Is it ok to check only debug.cpp?
I think manager.cpp's functions (launchNewModules etc) are used 🤔

And other modules in behavior veclocity planner has same structure and fuctions. Do you have any idea why only no_stopping_area module got the unusedFunction error?

[veqcc]

@kosuke55

thanks! Is it ok to check only debug.cpp?
I think manager.cpp's functions (launchNewModules etc) are used 🤔

You are right 👍

And other modules in behavior veclocity planner has same structure and fuctions. Do you have any idea why only no_stopping_area module got the unusedFunction error?

Actualy, there are similar unusedFunction warnings in other modules.
Since this is a PR for behavior_velocity_no_stopping_area_module, I mentioned only related warnings.
If you prefer not to delete these functions, I think it also makes sense.

[kosuke55]

@veqcc
It seem to get this warning about functions that is overridden. And I checked function in debug.cpp is also used. So we need to keep those functions.

[kosuke55]

LGTM!

[veqcc]

I will fix the build warning later

[veqcc]

@kosuke55
Hi, I have one question on this commit:
9e76436

Here, I have deleted dist_from_area_sum variable since it is only update inside the first if branch but soon breaks the loop.
Consequently, the parqameter margin is not necessary anymore, and in tern the following two function calls become totally the same https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43 .
Is it correct?

const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stuck_vehicle,
    planner_param_.detection_area_length);
const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stop_line,
    planner_param_.detection_area_length);
↓
const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, // margin is deleted
    planner_param_.detection_area_length);
const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, // margin is deleted
    planner_param_.detection_area_length);

[kosuke55]

@veqcc

Here, I have deleted dist_from_area_sum variable since it is only update inside the first if branch but soon breaks the loop.
Consequently, the parqameter margin is not necessary anymore, and in tern the following two function calls become totally the same https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43 .
Is it correct?

The change was made in the PR #6517 here and I checked with @kaigohirao san. dist_from_area_sum was already no longer needed, so you can delete them.

[veqcc]

@kosuke55
Thank you for your detailed investigation!!
I understand we can delete dist_from_area_sum from generateEgoNoStoppingAreaLanePolygon function!!

Now, my question is this:
The generateEgoNoStoppingAreaLanePolygon function does not need margin parameter anymore, and thus the following two variables stuck_vehicle_detect_area and stop_line_detect_area will become exactly the same. It is true?

  const Polygon2d stuck_vehicle_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stuck_vehicle,
    planner_param_.detection_area_length);
  const Polygon2d stop_line_detect_area = generateEgoNoStoppingAreaLanePolygon(
    *path, current_pose->pose, ego_space_in_front_of_stop_line,
    planner_param_.detection_area_length);
``
https://github.com/autowarefoundation/autoware.universe/blob/b6bd75073a61757b6212c578409363680df2ddd8/planning/behavior_velocity_no_stopping_area_module/src/scene_no_stopping_area.cpp#L153C3-L160C43

[veqcc]

@kosuke55
I have reverted the commit about dist_from_area_sum and created a new issue about it #7081 👍

[kosuke55]

@veqcc
We will work on the issue in other PR.
LGTM for your PR. Thank you!