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

Added support for LaneOffsetAction #702

Merged
merged 10 commits into from
Jan 12, 2021
Merged

Added support for LaneOffsetAction #702

merged 10 commits into from
Jan 12, 2021

Conversation

glopezdiest
Copy link
Contributor

@glopezdiest glopezdiest commented Dec 7, 2020

Description

This PR adds support for OSC LaneOffsetAction.

Added a new behaviours called ChangeActorLaneOffset to change the offset on runtime. Extended the LocalPlanner to be able to set an offset to the lateral controller (this CARLA's PR)

Some important notes:

  • Both the behavior and the controller might break if the offset is high enough to make the vehicle move outside the desired lane
  • Added a get_last_lane_offset_command to the actor_control.py, as if two LaneOffsetAction are triggered consecutively, the terminate of the first one will overwrite the initialization of the second behavior, setting it to 0, and not the desired value.

This file (LaneOffsetActionTest.zip) can be used to test the new behavior. It is a modification of the LaneChangeSimple.xosc but changing the lane change to two LaneOffsetActions

This PR will be ready when the changes to CARLA's local planner are in master (in other words, when 0.9.11 releases).
EDIT: With 0.9.11, this can now be merged into the main branch

Where has this been tested?

  • Platform(s): Ubuntu 18.04
  • Python version(s): 3.6
  • Unreal Engine version(s): 4.24
  • CARLA version: 0.9.10.1

Possible Drawbacks

None, as this is just an extension of the code


This change is Reviewable

@glopezdiest glopezdiest marked this pull request as ready for review January 7, 2021 08:47
Copy link
Collaborator

@fabianoboril fabianoboril left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 7 files at r1, 3 of 5 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @glopezdiest)


Docs/openscenario_support.md, line 120 at r3 (raw file):

| `ControllerAction`                                  | ✅            | ✅            | AssignControllerAction is supported, but a Python module has to be provided for the controller implementation, and in OverrideControllerValueAction all values need to be `False`. |
| `LateralAction`<br> `LaneChangeAction`             | &#10060;             | &#9989;            | Currently all lane changes have a linear dynamicShape, the dynamicDimension is defined as the distance and are relative to the actor itself (RelativeTargetLane).                  |
| `LateralAction`<br>`LaneOffsetAction`             | &#9989;             | &#10060;             |  Currently all type of dynamicShapes are ignored and depend on the controller. This action might not work as intended if the offset is high enough to make the vehicle exit its lane  |

Maybe mention that currently only the NPC controller supports the action.


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 50 at r3 (raw file):

        Update the plan (waypoint list) of the LocalPlanner
        """
        self._local_planner._waypoint_buffer.clear()    # pylint: disable=protected-access

This is required, if several plans are executed after each other. Was there a reason why you removed it?


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 61 at r3 (raw file):

        Update the plan (waypoint list) of the LocalPlanner
        """
        self._local_planner._vehicle_controller._lat_controller._offset = self._offset   # pylint: disable=protected-access

Can someone in CARLA please add getters. Having these private accesses is not a very good style.


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 98 at r3 (raw file):

        if self._offset_updated:
            self._offset_updated = False
            self._update_offset()

Can we add something to the other controllers to either support the offset, or at least provide a message that the offset is ignored?

Copy link
Contributor Author

@glopezdiest glopezdiest left a comment

Choose a reason for hiding this comment

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

Reviewable status: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @fabianoboril)


Docs/openscenario_support.md, line 120 at r3 (raw file):

Previously, fabianoboril (Fabian Oboril) wrote…

Maybe mention that currently only the NPC controller supports the action.

Added support for simple vehicle controller, which should be all of the lateral controller we have


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 50 at r3 (raw file):

Previously, fabianoboril (Fabian Oboril) wrote…

This is required, if several plans are executed after each other. Was there a reason why you removed it?

I removed this because this is already done when setting the global plan. If I recall correctly, the fact that we didn't clear the buffer was a bug we had at the local planner, which we fixed a while back. Most likely, we created the OSC controllers before fixing that, which is why that line was needed, but not anymore.


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 61 at r3 (raw file):

Previously, fabianoboril (Fabian Oboril) wrote…

Can someone in CARLA please add getters. Having these private accesses is not a very good style.

Agreed, we should do that. I'll do that after this PR


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 98 at r3 (raw file):

Previously, fabianoboril (Fabian Oboril) wrote…

Can we add something to the other controllers to either support the offset, or at least provide a message that the offset is ignored?

Added support for simple vehicle controller, which should be all of the lateral controller we have

Copy link
Collaborator

@fabianoboril fabianoboril left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @glopezdiest)


srunner/scenariomanager/actorcontrols/npc_vehicle_control.py, line 50 at r3 (raw file):

Previously, glopezdiest wrote…

I removed this because this is already done when setting the global plan. If I recall correctly, the fact that we didn't clear the buffer was a bug we had at the local planner, which we fixed a while back. Most likely, we created the OSC controllers before fixing that, which is why that line was needed, but not anymore.

Ah... Good to know. :)

Copy link
Collaborator

@fabianoboril fabianoboril left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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