Correct geodetic scale denominator calculation#3828
Correct geodetic scale denominator calculation#3828fdiazcarsi wants to merge 1 commit intomapfish:masterfrom
Conversation
|
Can you clear your pull request by doing an interactive rebase and removing all the extra commits? |
b2be240 to
4e511ee
Compare
|
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! |
|
Yes, thank you. |
|
@sbrunner @fdiazcarsi |
sebr72
left a comment
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pull request overview
Fixes an incorrect geodetic scale denominator computation in Scale#getGeodeticDenominator by correctly centering the geodetic measurement segment on the X coordinate of the provided map position.
Changes:
- Correct
minGeoXto useposition.x(instead ofposition.y) when computing the geodetic measurement segment’s start X.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| double width = 1; | ||
| double geoWidthInches = getResolutionInInches() * width; | ||
| double geoWidth = DistanceUnit.IN.convertTo(geoWidthInches, this.unit); | ||
| double minGeoX = position.y - (geoWidth / 2.0); | ||
| double minGeoX = position.x - (geoWidth / 2.0); | ||
| double maxGeoX = minGeoX + geoWidth; |
There was a problem hiding this comment.
This bug fix changes geodetic denominator behavior but there’s no regression test to prevent reintroducing the X/Y mix-up. Consider adding a unit test that calls getGeodeticDenominator() with a projection where distortion varies with easting (so swapping position.x and position.y would change the result), e.g., using CH1903 (EPSG:21781) with a coordinate where x != y and asserting the expected denominator.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@fdiazcarsi Without reply from you, and without sufficient test, we tried to get copilot to add some and it could not not). So we are closing this PR and creating #3979 |
This commit fixes a bug in the
getGeodeticDenominatormethod within theScaleclass.Problem:
The calculation for
minGeoX, which defines the starting X-coordinate of the horizontal line segment used for geodetic distance measurement, incorrectly usedposition.yinstead ofposition.x. This resulted in the geodetic calculation being performed at an incorrect location on the map, leading to an inaccurate scale denominator.Fix:
The
minGeoXcalculation has been corrected to useposition.x - (geoWidth / 2.0), ensuring that the horizontal line segment is properly centered around the providedposition'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.