Skip to content

Simplify scaleline calculation #4629

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

Merged
merged 4 commits into from
Jan 11, 2016
Merged

Conversation

ahocevar
Copy link
Member

@ahocevar ahocevar commented Jan 6, 2016

It looks like the scaleline control has some legacy code that is no longer needed now that the projection provides a point resolution in crs units.

Fixes #4572.
Fixes #3444.

@flipper44
Copy link

capture3
capture
This is a picture of a one square mile block and a resulting metric scaleline, after zooming the scaleline crashes. The map is using a us-ft projection. All layers Bing ESRI etc are reprojecting fine.
I do not get a crash when using metric projection.
I will take a look at where it is breaking

@ahocevar
Copy link
Member Author

ahocevar commented Jan 6, 2016

Could you please provide more context @flipper44, i.e. the code or a JSFiddle you used to produce the above image?

@flipper44
Copy link

Its hard to show the Ful;l code because it is dynamically built.
Here is what I found when using this Projection WKT
WKT ="PROJCS["IN83-EF",GEOGCS["LL83",DATUM["NAD83",SPHEROID["GRS1980",6378137.000,298.25722210]],PRIMEM["Greenwich",0],UNIT["Degree",0.017453292519943295]],PROJECTION["Transverse_Mercator"],PARAMETER["false_easting",328083.333],PARAMETER["false_northing",820208.333],PARAMETER["scale_factor",0.999966666667],PARAMETER["central_meridian",-85.66666666666670],PARAMETER["latitude_of_origin",37.50000000000000],UNIT["Foot_US",0.30480060960122]]";
proj4.defs('Custom', Wkt);
var x = proj4.defs('Custom');
var newProj = ol.proj.get('Custom');

Everything works up until my first Zoom (scale Bar values are still wrong) because viewState.Resolution is NaN PS this also causes ZoomSlider to crash

if I set newProj.units_ = "us-ft";
then it works (still bad scale values)
Edit:
I have found that I must do this because the value for that comes from proj4js and OL is "foot_us" and not "us-ft"..This WKT came from a Mapguide Runtimemap

@flipper44
Copy link

It looks like I will have to manually handle units coming from mapguide for now.

The Fix for bad Scale Values:
I Havent tested it fully but it would seem this line should be changed from
projection.getPointResolution(viewState.resolution, center) /
metersPerUnit;
to
projection.getPointResolution(viewState.resolution, center) *
metersPerUnit;

@ahocevar
Copy link
Member Author

ahocevar commented Jan 6, 2016

You're right @flipper44, the point resolution needs to be multiplied by the meters per unit. I made that change. Now I'll create an example with your WKT def to see if I can reproduce what else is wrong.

@ahocevar
Copy link
Member Author

ahocevar commented Jan 6, 2016

@flipper44 513cf13 fixes your other issue. I also added an example.

This is now ready for review.

projection: 'Indiana-East',
center: ol.proj.fromLonLat([-85.685, 39.891], 'Indiana-East'),
zoom: 7,
minZoom: 4
Copy link
Member

Choose a reason for hiding this comment

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

My computer stops working when I get to 5. Might be worth limiting this to 6.

Copy link
Member

Choose a reason for hiding this comment

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

Could also be that enforcing a center constraint with extent would help.

@tschaub
Copy link
Member

tschaub commented Jan 7, 2016

image

Looks like there is an issue when setting units: 'degrees'.

Really nice to see the removal of the unnecessary code here.

@flipper44
Copy link

Re-projection on smaller local coordinate systems does get odd when zOomed way past the local extents.
Also I think there was a reason I had an if statement for degrees in my initial example

@ahocevar
Copy link
Member Author

ahocevar commented Jan 7, 2016

Zoom constraint changed, center constraint introduced, degree issue fixed.

@fredj fredj added this to the v3.13.0 milestone Jan 7, 2016
@ahocevar ahocevar force-pushed the scaleline branch 2 times, most recently from b9d3716 to 192e7bc Compare January 7, 2016 19:12
@fredj
Copy link
Member

fredj commented Jan 11, 2016

LGTM

ahocevar added a commit that referenced this pull request Jan 11, 2016
Simplify scaleline calculation
@ahocevar ahocevar merged commit d916e8c into openlayers:master Jan 11, 2016
@ahocevar ahocevar deleted the scaleline branch January 11, 2016 09:12
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.

4 participants