Skip to content

Conversation

@fdiazcarsi
Copy link

This commit fixes a bug in the getGeodeticDenominator method within the Scale class.

Problem:
The calculation for minGeoX, which defines the starting X-coordinate of the horizontal line segment used for geodetic distance measurement, incorrectly used position.y instead of position.x. This resulted in the geodetic calculation being performed at an incorrect location on the map, leading to an inaccurate scale denominator.

Fix:
The minGeoX calculation has been corrected to use position.x - (geoWidth / 2.0), ensuring that the horizontal line segment is properly centered around the provided position's X-coordinate.

Impact:
This fix ensures that geodetic scale denominators are calculated accurately, improving the precision of map rendering and scale representation, especially for projections where geodetic corrections are critical.

@sbrunner
Copy link
Member

sbrunner commented Nov 4, 2025

Can you clear your pull request by doing an interactive rebase and removing all the extra commits?

@fdiazcarsi fdiazcarsi force-pushed the fix/correct-geodetic-scale-denominator-calculation branch from b2be240 to 4e511ee Compare November 6, 2025 07:48
@fdiazcarsi
Copy link
Author

Hi! As requested, I've cleaned up the PR history using an interactive rebase. It should now contain only the relevant commit on top of the latest master. Ready for review again. Thanks!

Copy link
Member

@sbrunner sbrunner left a comment

Choose a reason for hiding this comment

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

@sebr72 does it look good to you?

@fdiazcarsi
Copy link
Author

Yes, thank you.

@sbrunner sbrunner added backport 3.33 Add this label to backport the pull request to the '3.33' branch backport 3.28 Backport the pull request to the '3.28' branch backport 3.29 Backport the pull request to the '3.29' branch backport 3.30 Backport the pull request to the '3.30' branch backport 3.31 Backport the pull request to the '3.31' branch backport 3.32 Add this label to backport the pull request to this branch labels Nov 6, 2025
@sebr72
Copy link
Contributor

sebr72 commented Nov 6, 2025

@sbrunner
I agree with the intent of the PR. But what really puzzles me is the fact that none of the tests failed with such change.

@fdiazcarsi
Thanks for spotting and fixing this.
But since this PR is intended to be deployed to 6 versions, I believe that this kind of change must be tested.

@sebr72 sebr72 self-requested a review November 7, 2025 08:48
Copy link
Contributor

@sebr72 sebr72 left a comment

Choose a reason for hiding this comment

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

As a general practice, we always add tests when making changes. But in this case in particular (as it appears to be a very old bug - and sometimes they are not), please create at least a Unit test if CI test is not required: Your decision

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 3.28 Backport the pull request to the '3.28' branch backport 3.29 Backport the pull request to the '3.29' branch backport 3.30 Backport the pull request to the '3.30' branch backport 3.31 Backport the pull request to the '3.31' branch backport 3.32 Add this label to backport the pull request to this branch backport 3.33 Add this label to backport the pull request to the '3.33' branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants