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

Adapt ReferenceLine ST definition to avoid transformation ambiguity and discontinuities #651

Conversation

ReinhardBiegelIntech
Copy link
Contributor

@ReinhardBiegelIntech ReinhardBiegelIntech commented Jul 8, 2022

This addresses #645

Current definition of s,t coordinates has some (well documented) downsides, which we would like to mitigate with this proposed change. With the updated s,t definition, x,y <-> s,t associations are unique inside the lane geometries and there are no gaps in the s coordinate anymore.

This approach is used and tested by openPASS .

Checks will follow (but the links to howtocontribute.html seem to be dead... where to get this information?)

  • My suggestion follows the style and contributors guidelines.
  • I have taken care about the documentation.
  • I have done the DCO signoff.
  • My changes generate no errors when passing CI tests.
  • I have successfully implemented and tested my fix/feature locally.
  • Appropriate reviewer(s) are assigned.

@ArunDas1
Copy link

ArunDas1 commented Jul 8, 2022

@tbleher @ThomasNaderBMW @jdsika Here is our proposal regarding the discussed issue with computation of s-t-coordinates.

osi_referenceline.proto Outdated Show resolved Hide resolved
// and two points (P1 and P2) not part of the reference line.
//
// Calculation of ST for P1:
// - Calculate the instersection point I of the T axes of R0 and R1.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/instersection/intersection/

osi_referenceline.proto Show resolved Hide resolved
//
// Calculation of ST for P1:
// - Calculate the instersection point I of the T axes of R0 and R1.
// - As P1 lies in the sector defined by these T axes it is considered part
Copy link
Contributor

Choose a reason for hiding this comment

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

This change removes the \c formatting markers. I found these helpful for readability when reading the rendered document (as will be displayed on the website). I suggest you check how it looks in Doxygen, and probably re-add the \c formatting markers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, these should be added again.

@tbleher
Copy link
Contributor

tbleher commented Sep 16, 2022

Change looks good to me.

@ReinhardBiegelIntech
Copy link
Contributor Author

As discussed in the ASAM OSI Other Models Workgroup meeting on Monday, we updated the PR to clarify the constraints on the T axis.

@ThomasNaderBMW ThomasNaderBMW added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Oct 10, 2022
@ThomasNaderBMW ThomasNaderBMW added this to the V4.0.0 milestone Oct 10, 2022
@jdsika jdsika added the Harmonisation The Group in the ASAM development project working on harmonisation with other standards. label Oct 12, 2022
@thomassedlmayer thomassedlmayer removed the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Oct 12, 2022
@lemmer-fzi
Copy link
Contributor

After reading the documentation and having a look at the provided diagrams i have a question:
Consider a reference line that only has two points (straight line). How would you calculate the (s,t) coordinates for a point that lies on a line perpendicular to the last point of the segment. As far as i can, using the proposed definition of the t axis their is no way to define a t-axis that crosses the point and still maps it to a valid s-coordinate (a value of s smaller than the maximal value specified by the last point on the reference line).
In large, this question also applies to the last segment of a specified reference line. Could you maybe elaborate on this?

@weissdavid
Copy link
Contributor

After reading the documentation and having a look at the provided diagrams i have a question: Consider a reference line that only has two points (straight line). How would you calculate the (s,t) coordinates for a point that lies on a line perpendicular to the last point of the segment. As far as i can, using the proposed definition of the t axis their is no way to define a t-axis that crosses the point and still maps it to a valid s-coordinate (a value of s smaller than the maximal value specified by the last point on the reference line). In large, this question also applies to the last segment of a specified reference line. Could you maybe elaborate on this?

You can always define the t-axis to be the line perpendincular to the segment. In this case your point, which is perpendicular to the last point lies exactly on this t-axis and therefore has the same s coordinate (the t-axis consists of all points with equal s coordinate). Furthermore all s coordinates are valid (even negative values).

@ReinhardBiegelIntech
Copy link
Contributor Author

@ThomasNaderBMW
We updated the PR as discussed last week (addition of a reference line type enum).

Copy link
Contributor

@tbleher tbleher left a comment

Choose a reason for hiding this comment

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

I didn't have time to participate in the discussion, so the following review is purely based on reading the patchset. This seems fine for keeping backward compatibility (but also means that all the old documentation will have to be kept), but also means that for agent models, it now becomes MUCH harder to support this, since they will have to implement TWO modes for ST calculations instead of one (simulation environments need only support one, but agent models need to support two). So I don't really like this idea.

Personally, I would be OK with breaking backward compatibility here (considering this is a very new feature, and probably noone has implemented it fully), but I don't know if this will gain consensus.

If the two modes are kept, then please at least mark one of the modes as deprecated, and advise agent models to only support the newer modes. If some agent models only support the first mode, and some only support the second mode, then no interoperability is achieved!

osi_referenceline.proto Outdated Show resolved Hide resolved
osi_referenceline.proto Show resolved Hide resolved
osi_referenceline.proto Outdated Show resolved Hide resolved
osi_referenceline.proto Outdated Show resolved Hide resolved
@weissdavid
Copy link
Contributor

OSI_s-t_calculation.pdf
As discussed in the CCB meeting, we prepared an exemplary calculation.

@thempen
Copy link
Contributor

thempen commented Nov 9, 2022

From WG Harmonization: Please add a hint at the beginning, that the ST coordinate definition should not be misunderstood as the ST definition in OpenDRIVE and OpenSCENARIO 1.x.

The current proto definition for the enum fields "other" and "unknown" are missing, so it does not follow the coding convention guidelines: https://opensimulationinterface.github.io/osi-documentation/#_contributing_changes
To be determined by CCB if this is an exceptional situation and can be accepted.

@ReinhardBiegelIntech
Copy link
Contributor Author

@thempen Added a note as discussed. Should this be tagged as ReadyForCCBReview again?

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Nov 9, 2022
@pmai
Copy link
Contributor

pmai commented Nov 21, 2022

OSI CCB 2022-11-21: Can be merged as is with necessary changes to resolve technical conflicts. Merge and cleanup done by @pmai.

@pmai pmai added ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB. and removed ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. labels Dec 12, 2022
…d discontinuities

Also-by: Weiss David <david.weiss@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Also-by: Weiss David <david.weiss@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
@pmai pmai force-pushed the improve-s-t-calculation branch from e85fedb to 8d41af2 Compare December 12, 2022 15:01
The type of a reference line will determine how S and T coordinates have
to be calculated. Type POLYLINE is equal to the previous ST definition.
A new type POLYLINE_WITH_T_AXIS is introduced for improved ST handling.

Signed-off-by: Weiss David <david.weiss@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Also-by: Weiss David <david.weiss@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
Also-by: Weiss David <david.weiss@in-tech.com>
Signed-off-by: Reinhard Biegel <reinhard.biegel@in-tech.com>
@pmai pmai force-pushed the improve-s-t-calculation branch from 8d41af2 to 79e3994 Compare December 12, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Harmonisation The Group in the ASAM development project working on harmonisation with other standards. Other Models ReadyToMerge This PR has been approved to merge and will be merged by a member of the CCB.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants