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

RJD-1334/fix_longitudinal_distance #1403

Merged
merged 29 commits into from
Nov 7, 2024
Merged

Conversation

robomic
Copy link
Contributor

@robomic robomic commented Oct 1, 2024

Abstract

This pull requests fixes issue described here

Details

  1. Simplification of HdMapUtils::getLongitudinalDistance function logic, while retaining its functionalify.
  2. Additional matching: previously matching occurred at the starts of the lanelets, now matching is also performed at 0.5 * min(from_lanelet, to_lanelet) to cover the corner case (please see the mentioned issue description).
  3. Additional unit tests for the modified function.
  4. During development i have decided that using the first instead of the second approach is far superior (refer to this discussion)
  • This approach produces cleaner code in general.
  • This approach does not require modification of the underlying centerline's math::geometry::CatmullRomSpline.
  • The working POC of the first approach is still implemented in the first commit of this PR.
  1. Docs have been updated accordingly.
  2. Some recurring scenario conditions have been updated. Currently, majority of these conditions are too precise, this disallows any changes in the calculations.

References

#1364
Jira ticket

Copy link

github-actions bot commented Oct 1, 2024

Checklist for reviewers ☑️

All references to "You" in the following text refer to the code reviewer.

  • Is this pull request written in a way that is easy to read from a third-party perspective?
  • Is there sufficient information (background, purpose, specification, algorithm description, list of disruptive changes, and migration guide) in the description of this pull request?
  • If this pull request contains a destructive change, does this pull request contain the migration guide?
  • Labels of this pull request are valid?
  • All unit tests/integration tests are included in this pull request? If you think adding test cases is unnecessary, please describe why and cross out this line.
  • The documentation for this pull request is enough? If you think adding documents for this pull request is unnecessary, please describe why and cross out this line.

@robomic robomic self-assigned this Oct 1, 2024
@robomic robomic changed the title Fix/longitudinal distance RJD-1334 Oct 1, 2024
@robomic robomic changed the title RJD-1334 RJD-1334/fix_longitudinal_distance Oct 1, 2024
@robomic robomic added the bump minor If this pull request merged, bump minor version of the scenario_simulator_v2 label Oct 8, 2024
@robomic
Copy link
Contributor Author

robomic commented Oct 23, 2024

Regression tests: OK

@robomic robomic marked this pull request as ready for review October 23, 2024 13:32
Copy link
Collaborator

@hakuturu583 hakuturu583 left a comment

Choose a reason for hiding this comment

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

Code is looks good to me and I confirmed this pull request has no-regression.
Please add one comment addition.

Copy link

sonarcloud bot commented Nov 6, 2024

@hakuturu583 hakuturu583 merged commit 50bb2db into master Nov 7, 2024
13 checks passed
@github-actions github-actions bot deleted the fix/longitudinal_distance branch November 7, 2024 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor If this pull request merged, bump minor version of the scenario_simulator_v2 wait for regression test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants