fix(LibCarla): port lane marking, waypoint loop, lane corners, and pugixml null fixes#9652
fix(LibCarla): port lane marking, waypoint loop, lane corners, and pugixml null fixes#9652youtalk wants to merge 3 commits intocarla-simulator:ue5-devfrom
Conversation
…l null fixes Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Ports several UE4-dev bug fixes into LibCarla, focusing on preventing crashes/infinite loops and improving robustness in lane marking generation and XML node insertion.
Changes:
- Add
_rootnull-guards to multiple pugixml insertion APIs and introduce GTest coverage. - Refactor lane marking generation to use a helper that avoids divide-by-zero on zero-width lanes.
- Prevent waypoint traversal infinite recursion and harden lane corner computations (clamping
s).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| LibCarla/source/third-party/pugixml/pugixml.cpp | Adds _root null checks to avoid crashes during insertion operations. |
| LibCarla/source/test/common/test_pugixml.cpp | Introduces GTests intended to validate the pugixml null-guard behavior. |
| LibCarla/source/carla/road/MeshFactory.h | Declares a helper for lane marking edge computation. |
| LibCarla/source/carla/road/MeshFactory.cpp | Uses the helper to generate lane marking geometry and implements robust edge computation. |
| LibCarla/source/carla/road/Map.cpp | Adds loop-breaking logic in waypoint traversal and a guard for degenerate edges in tree placement. |
| LibCarla/source/carla/road/Lane.cpp | Clamps s in GetCornerPositions to avoid out-of-range crashes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::pair<geom::Vector3D, geom::Vector3D> edges = | ||
| ComputeEdgesForLanemark(lane_section, lane, s_current, lane_mark_info.width); |
There was a problem hiding this comment.
This refactor changes behavior for callers that previously used GetCornerPositions(s_current, 0) (e.g., solid lane marking cases) by always using road_param.extra_lane_width inside ComputeEdgesForLanemark. That can shift lane marking geometry relative to the lane border. Consider adding an extra_width (or equivalent) parameter to ComputeEdgesForLanemark and pass the original values (0 vs road_param.extra_lane_width) from each call site.
There was a problem hiding this comment.
Good catch. Confirmed: the old Solid case and the final-segment call passed extra_width = 0, while the Broken cases used road_param.extra_lane_width. The refactor unifies them on road_param.extra_lane_width, which silently shifts solid lane marking positions on maps with non-zero extra lane width. To preserve the original behavior exactly, please add an extra_width parameter to ComputeEdgesForLanemark and forward 0 from the call sites that previously used it.
Probably the best action right now is to create a new issue ticket and track this possible issue separately.
| std::pair<geom::Vector3D, geom::Vector3D> edges = | ||
| lane.GetCornerPositions(s_current, road_param.extra_lane_width); |
There was a problem hiding this comment.
This refactor changes behavior for callers that previously used GetCornerPositions(s_current, 0) (e.g., solid lane marking cases) by always using road_param.extra_lane_width inside ComputeEdgesForLanemark. That can shift lane marking geometry relative to the lane border. Consider adding an extra_width (or equivalent) parameter to ComputeEdgesForLanemark and pass the original values (0 vs road_param.extra_lane_width) from each call site.
There was a problem hiding this comment.
Same case as the previous comment.
JesusAnaya
left a comment
There was a problem hiding this comment.
This change has one potential bug (LibCarla/source/carla/road/Map.cpp:1294-1306). Please review my comments.
| std::pair<geom::Vector3D, geom::Vector3D> edges = | ||
| ComputeEdgesForLanemark(lane_section, lane, s_current, lane_mark_info.width); |
There was a problem hiding this comment.
Good catch. Confirmed: the old Solid case and the final-segment call passed extra_width = 0, while the Broken cases used road_param.extra_lane_width. The refactor unifies them on road_param.extra_lane_width, which silently shifts solid lane marking positions on maps with non-zero extra lane width. To preserve the original behavior exactly, please add an extra_width parameter to ComputeEdgesForLanemark and forward 0 from the call sites that previously used it.
Probably the best action right now is to create a new issue ticket and track this possible issue separately.
| std::pair<geom::Vector3D, geom::Vector3D> edges = | ||
| lane.GetCornerPositions(s_current, road_param.extra_lane_width); |
There was a problem hiding this comment.
Same case as the previous comment.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Invert the degenerate-edges guard so that only tree placement is skipped when edges.first == edges.second, instead of using continue which skipped the s_current increment and caused an infinite loop. Signed-off-by: Yutaka Kondo <yutaka.kondo@youtalk.jp>
Description
Port of 4 ue4-dev bug fixes:
Lane marking generation
(
86fcfe5e8) — ExtractComputeEdgesForLanemarkhelper to handle zero-width lanes without divide-by-zero. Addedges.first == edges.secondguard in tree generation.Waypoint next/previous infinite loop
(
e41ffa03c) — Prevent infinite recursion inGetNext/GetPreviouswhen two roads with opposite directions are connected as each other's successor.Lane corners crash (
4d4dd1d11) — Clampsparameter inGetCornerPositionsto[0, road_length]to prevent crash when requested s exceeds lane distance.pugixml null pointer dereference
(
4ca87e818+6cb77140c) — Add!_rootnull checks in 6 xml_node insertion methods to prevent crashes on empty nodes. Includes Google Test cases.Where has this been tested?
Possible Drawbacks
None. All changes are defensive guards and bug fixes.
This change is