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

Better getPointResolution default when no transform available #11811

Merged
merged 5 commits into from Dec 13, 2020

Conversation

mike-000
Copy link
Contributor

@mike-000 mike-000 commented Dec 7, 2020

The default getPointResolution function does not give a meaningful result when proj4 is not used and there is no EPSG:4326 transform defined (as can be seen from the scalebar in https://codesandbox.io/s/wild-resonance-9350t) as the default identity transform is only meaningful when both projections units are degrees. This change is equivalent to having specified
getPointResolution: function (resolution) { return resolution; }
in the Projection options in that situation

Added a ScaleLine to the WMS without Projection example as it is now meaningful, although it may lack the 100% accuracy of the Single Image WMS with Proj4js or Custom Tiled WMS examples.

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.

Not proceeding with transforms when toEPSG4326 equals the identityTransform is a good change, but I wonder why the whole change would not be as simple as

--- a/src/ol/proj.js
+++ b/src/ol/proj.js
@@ -202,16 +202,20 @@ export function getPointResolution(projection, resolution, point, opt_units) {
     }
   } else {
     const units = projection.getUnits();
-    if ((units == Units.DEGREES && !opt_units) || opt_units == Units.DEGREES) {
+    const toEPSG4326 = getTransformFromProjections(
+      projection,
+      get('EPSG:4326')
+    );
+    if (
+      toEPSG4326 === identityTransform ||
+      (units == Units.DEGREES && !opt_units) ||
+      opt_units == Units.DEGREES
+    ) {
       pointResolution = resolution;
     } else {
       // Estimate point resolution by transforming the center pixel to EPSG:4326,
       // measuring its width and height on the normal sphere, and taking the
       // average of the width and height.
-      const toEPSG4326 = getTransformFromProjections(
-        projection,
-        get('EPSG:4326')
-      );
       let vertices = [
         point[0] - resolution / 2,
         point[1],

Especially since you said

This change is equivalent to having specified
getPointResolution: function (resolution) { return resolution; }
in the Projection options in that situation

@mike-000
Copy link
Contributor Author

That would not handle the case where opt_units is not degrees but is different to the projection units.

@ahocevar
Copy link
Member

In this case I'd need tests to see what your intention and the expected results are.

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, @mike-000

@ahocevar ahocevar merged commit 49f03cf into openlayers:main Dec 13, 2020
@mike-000 mike-000 deleted the patch-6 branch December 30, 2020 00:20
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.

None yet

2 participants