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

Implements inertial_to_backend_frame_translation #35

Merged

Conversation

agalbachicar
Copy link
Collaborator

Pairs with maliput/maliput#401

Copy link
Collaborator

@liangfok liangfok left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -69,20 +69,25 @@ api::LanePosition Lane::DoEvalMotionDerivatives(const api::LanePosition&, const
}

api::InertialPosition Lane::DoToInertialPosition(const api::LanePosition& lane_pos) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am a little confused here. The docstring in https://github.com/ToyotaResearchInstitute/maliput/pull/401/files indicates that this method should only be overriden when "the Inertial and Backend Frames do not differ". Yet, the use of segment()->junction()->road_geometry()->inertial_to_backend_frame_translation() indicates that they may differ.

@liangfok
Copy link
Collaborator

liangfok commented Mar 9, 2021

Per discussion during meeting just now, if we provide dragway-specific support for an inertial-to-backend transform, it should have no run-time overhead. That is, the in-memory representation should be modified to account for the inertial frame. Thus, the only overhead should be at RoadNetwork initialization time.

@agalbachicar
Copy link
Collaborator Author

@liangfok note that dragway does not use geometry_base. All the linkage is managed by dragway::RoadGeometry constructor so we are only constrained by the API. I could revert this PR and just implement a stub RoadGeometry::inertial_to_backend_frame_translation() if that's more appropriate (same proposed solution in maliput_multilane --> maliput/maliput_multilane#46).

What do you think?

@agalbachicar agalbachicar merged commit 8728ae1 into master Mar 11, 2021
@agalbachicar agalbachicar deleted the agalbachicar/#400_internal_maliput_inertial_frame branch March 11, 2021 19:54
@agalbachicar
Copy link
Collaborator Author

We'll keep it as is and change it if required in the future.

@liangfok
Copy link
Collaborator

Okay.

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