fix(topic remapping): a bug of remapping regarding goal_pose#196
fix(topic remapping): a bug of remapping regarding goal_pose#196xtk8532704 merged 2 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the topic remapping logic where the goal_pose configuration check was incorrectly triggering even when the value was an empty string representation "{}". The fix adds an additional condition to properly validate the goal_pose configuration before applying remapping.
- Updates the conditional check to exclude empty string representations of dictionaries
- Ensures remapping only occurs when
goal_posehas a meaningful value
| add_remap("/localization/kinematic_state", remap_list) | ||
| add_remap("/localization/acceleration", remap_list) | ||
| if conf.get("goal_pose") is not None: | ||
| if conf.get("goal_pose") is not None and conf["goal_pose"] != "{}": |
There was a problem hiding this comment.
The hardcoded string "{}" comparison is fragile and could break if the empty value format changes. Consider using a more robust check like not conf["goal_pose"].strip() or defining a constant for the empty value pattern.
| if conf.get("goal_pose") is not None and conf["goal_pose"] != "{}": | |
| if conf.get("goal_pose") is not None and conf["goal_pose"].strip() != "": |
MasatoSaeki
left a comment
MasatoSaeki
left a comment
There was a problem hiding this comment.
It is okay to use only if conf["goal_pose"] != "{}": I think. What do you think?
|
I‘m not sure if there are some cases where the |
I think this process is done everytime, so I agree with @MasatoSaeki 's comment. |
Signed-off-by: xtk8532704 <1041084556@qq.com>
|
Fixed in a safe manner. |
Types of PR
Description
The original remapping doesn't work as expected because the
conf[goal_pose]may not be None but a string{}inplanning_controlorlocalizationscenariosThis PR fixes this problem.
How to review this PR
Others