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

more precise angle computation #1205

Merged
merged 2 commits into from
Oct 20, 2024
Merged

Conversation

leyan
Copy link
Contributor

@leyan leyan commented Oct 15, 2024

Currently, when using "symbol-placement": "line" and "icon-rotation-alignment": "map", the angle of the icon is computed in ol-mapbox-style, as we know the location of the icon placement but not the corresponding line segment information. So the computation first tries to determine on which line segment the icon is located, then computes the slope of this segment and uses that as the rotation of the icon.

However, the algorithm used to determine on which segment is the icon is too simplistic: it simply takes the first segment for which the icon is inside the bounding box. However, there is no guarantee that this segment is the correct one and it is quite easy to construct a counter-example giving a wrong rotation angle:
image

while it should be

image

This pull request proposes to replace this bounding box by a more precise comparison. I used to 0.001 tolerance to avoid dealing with potential floating point unpleasantness, I don't know if it is the ideal value but it is anyway much more precise than the current version.

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Going forward, I think a placement: line option for ol/style/Image would make sense, to avoid the potentially expensive calculation here. But for now, your code makes sense. Please fix the linter errors though.

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

Thanks, @leyan

@ahocevar ahocevar merged commit ef96821 into openlayers:main Oct 20, 2024
1 check passed
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