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

Add option “usedLevelForFractional” for GridLayer. #6034

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

boundaryfree
Copy link

No description provided.

Andy Zhuang added 2 commits January 31, 2018 10:51
Sometimes we want to use the upper level image for fractional level
tile. This can give the high quality of image in some industries, for
example CAD drawing.
@ghybs
Copy link
Collaborator

ghybs commented Feb 1, 2018

Hi,

Thank you for proposing your first PR here!

Please can you kindly provide some explanation on the reason why you think this PR is useful for Leaflet users?
While we can guess from the code what it does, having context greatly helps in making sure the way the PR is implemented is relevant, and it also helps history.

Additionally, note that Leaflet provides some tests that need to pass before we can merge a PR.
In this case, the lint step triggered some style-related errors.
Please could you kindly fix those?

We would also highly appreciate that you include a unit test related to the new feature that you introduce.

Keep up the good work!

@YorkshireChemist
Copy link

Just to say that I believe this will be very useful for some Leaflet users. A number of people (e.g., at stackoverflow) mentioned that Leaflet images are blurred at non-fractional zoom. This is particularly important for people using Leaflet to display high-quality non-geographic images (e.g., pyramids via Zoomify or DeepZoom plugins). Clamping zoom at the ceiling solves this problem, and I have seen some already modifying Leaflet core along the lines of the proposed PR.

@ghybs
Copy link
Collaborator

ghybs commented Dec 6, 2018

Hi @YorkshireChemist,

Thank you for your input!

Please feel free to open a proper issue with detailed description of the use case, and in particular linking the Stack Overflow questions you mention.
That would make a very good reference for the usage of this feature.

Please feel free also to improve this PR, in particular to fix the style and potentially affected test(s).

@YorkshireChemist
Copy link

Hi @ghybs ,

Thanks, will do. It might take a little while but I will do it.

YorkshireChemist added a commit to YorkshireChemist/Leaflet that referenced this pull request Dec 31, 2018
This is a remake of PR Leaflet#6034 by @boundaryfree. At fractional zoom levels, tiles have to be either scaled up or down. Currently, Leaflet scales to the nearest integer zoom which means that the tiles can be scaled up leading to blurry images. This is particularly important for non-geographic images, e.g., when Leaflet is used to display pyramids with the Zoomify or Deep Zoom plugins - the blurring is quite noticeable for high-quality images such as photographs! It can also important for vector drawings such as CAD drawings.

This PR adds an option for the user to specify whether to scale the tiles at fractional zoom to the nearest integer zoom (default), or always scale them down from the higher integer zoom (this gives sharper images but increases download size).

The issue of blurred tiles in Leaflet has been discussed a few times before, e.g.: 
https://stackoverflow.com/questions/40387914/force-leaflet-map-to-use-only-integer-zoom-levels-no-fractional-levels
Leaflet#6068
Leaflet#6069

Here is a relevant detailed discussion of the same issue in mapbox:
mapbox/mapbox-gl-js#1030
if (options.usedLevelForFractional === 'round') {
return this._clampZoom(Math.round(zoom));
}
else if (options.usedLevelForFractional === 'ceil') {
Copy link

Choose a reason for hiding this comment

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

A quick grep shows that Leaflet repo seems to use else ifs/elses on the same line as the previous block's closing brace so you might want to fix it so that it won't prevent taking this in (I need this very same feature myself :-)).

keepBuffer: 2,

// @option usedLevelForFractional: String = 'round'|'ceil'
// Determin which level (upper or lower) image to use for the fractional level.
Copy link

Choose a reason for hiding this comment

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

Determine

@@ -543,8 +547,21 @@ export var GridLayer = Layer.extend({
return zoom;
},

_usedZoom: function(zoom) {
Copy link

Choose a reason for hiding this comment

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

Space before opening parenthesis

@ij1
Copy link

ij1 commented Feb 20, 2019

My take on this same feature is in #6505.

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

Successfully merging this pull request may close these issues.

None yet

5 participants