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

Feature/proj offset #794

Merged

Conversation

thomassedlmayer
Copy link
Contributor

This PR adds the PROJ offset mechanism as already defined in OpenDrive. See OpenDrive Specification.

@thomassedlmayer thomassedlmayer added the Harmonisation The Group in the ASAM development project working on harmonisation with other standards. label Mar 22, 2024
@thomassedlmayer thomassedlmayer added this to the V3.7.0 milestone Mar 22, 2024
@thomassedlmayer thomassedlmayer requested a review from thempen March 22, 2024 09:16
@thomassedlmayer thomassedlmayer marked this pull request as ready for review March 22, 2024 09:22
Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

Looks good to me, minor formatting fix applied, don't know about the complexity of the yaw but have no opinion on it either.

doc/architecture/reference_points_coordinate_systems.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@thempen thempen left a comment

Choose a reason for hiding this comment

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

I Think this proposal is very good. I haven't seen any direct mistakes. As my only remark: I stumbled over the "xOSI" vs the "xWorld" definition. We have many definition of data in world coordinates within the whole OSI standard. Shouldn't they be renamed to xOSI then? Otherwise the formula should be "xWorld = xWorld * cos(yaw) - xWorld * sin(yaw) + xOffset", where xWorld will be with the ProjFramOffset, if one is available. All this naturally applies to the y coordinate as well.

Copy link
Contributor

@thempen thempen left a comment

Choose a reason for hiding this comment

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

The PR is clearly understandable and good.
The Workgroup Harmonization & Other Models approves the PR and suggests, that the usage of "World" in other OSI messages shoud be reworked and clarified, if neccessary.

@thempen thempen added the ReadyForCCBReview Indicates that this MR is ready for a final review and merge by the CCB. label Mar 25, 2024
@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 Mar 25, 2024
@pmai
Copy link
Contributor

pmai commented Mar 25, 2024

CCB 2024-03-25: Merge as-is, handle world coordinate system confusions in separate PR for issue #796.

@pmai pmai force-pushed the feature/proj_offset branch from 417de90 to 274859c Compare March 25, 2024 11:53
@pmai pmai merged commit c9053e5 into OpenSimulationInterface:master Mar 25, 2024
5 checks passed
@jdsika
Copy link
Contributor

jdsika commented Apr 24, 2024

Reviewed for v3.7.0

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. 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.

4 participants