publiclab / Leaflet.DistortableImage

A Leaflet extension to distort or "rubber sheet" images
https://publiclab.github.io/Leaflet.DistortableImage/examples/
BSD 2-Clause "Simplified" License
273 stars 284 forks source link

images distort while scaling #85

Open jywarren opened 6 years ago

jywarren commented 6 years ago

via @patcoyle in https://github.com/publiclab/mapknitter/issues/195 - copying this over!

Plus LOTS of detailed info on this bug over here: https://github.com/publiclab/mapknitter/issues/153


See attached screenshot of in-progress map: http://mapknitter.org/maps/livermore-ca-mobius-with-8mm-lens Image at bottom right is example where when attempting to scale, size, or rotate, the image distorts. Sometimes, to trapezoidal shape, sometimes "squashed" or stretched in one dimension.

what you did just before the problem occurred: Observed problem previously in Safari 8.07, Firefox 39, so tried Chrome 44.0.2403.125.
your browser and browser version: Noted above
your operating system: OSX 10.10.4
any web addresses needed to see the problem (such as the URL of this page): 

http://mapknitter.org/maps/livermore-ca-mobius-with-8mm-lens your PublicLab.org username (if you have one): patcoyle describe how urgent the problem is (desperate or just-trying-to-help!): Mapknitter is mainline tool, so if systemic, urgent.

If you are able, it may also help to quickly resolve the issue if you:

test if it happens in a different browser or on another computer: Noted above
test if it happens whether you are logged in or not (if applicable): NA
test if it affects other users, or whether this happens to you only sometimes: Don't know

screenshot skewed image

ChrisLowe-Takor commented 6 years ago

I've a couple of observations on this. Using OSX and chrome I can reliably reproduce this on the example by:

Grabbing the top right handle and slowly dragging directly upwards you can create trapezoid distortion:

screen shot 2018-08-26 at 12 43 19 pm

By grabbing the top left handle and slowly dragging to the left you can stretch the image horizontally:

screen shot 2018-08-26 at 12 43 33 pm

And by grabbing any handle and very slowly rotating in either direction you can induce a little of both.

screen shot 2018-08-26 at 12 43 57 pm

Another thing to point out is the geographic size plays no role. It behaves the same big or small.

Here is where it gets interesting. When I was writing the react wrapper I found myself wanting to make small changes to either scale or rotate and the results were not what I was looking for so I split them into two seperate actions. The problem with the distortion was completely eliminated and personally I found it easier to use.

Might want to discuss with your users about changing core functionality like that but it will certainly eliminate the problem.

Anyway, the heart of the problem is here:

https://github.com/publiclab/Leaflet.DistortableImage/blob/master/src/edit/RotateHandle.js#L19-L20

This answer suggests scaling first then rotating which would be a cheap idea to try.

Another option would be to combine the matrix transformations as outlined here. The theory is that the rotate -> refresh -> scale -> refresh might be introducing errors because of the way its applied them to the DOM as style.

Third idea would be to put a threshold on the calculations or clamp them with .toFixed() to prevent floating point accuracy issues around cos(x) as x approaches 0. Speaking of floating points another avenue may be to use BigNumbers for the rotation calculations.

jywarren commented 6 years ago

This is really great research, thank you so much! I like your proposed solution of using .toFixed() or scaling first then rotating... worth a try for sure.

I mentioned in #105 but we might also have an options flag to switch between a separate vs. combined scale/rotate interface. This could solve the issue for lots of people and I'd happily accept a PR or work towards this goal and would love to support if anyone's interested.

sashadev-sky commented 5 years ago

@jywarren @ChrisLowe-Takor I have been working in this library for a bit now and just wanted to give an update on this issue! I think the problem was because we were applying transformations on the images assuming they are squares, but really they don't have the same height and width (difference of around 150px). By setting edgeMinWidth and edgeMinHeight options equal to their w/h when instantiating them and adding some conditional logic in the scaling class, scaling no longer creates distortions:

bug5

Please let me know if you have any thoughts on this!

jywarren commented 5 years ago

Amazing!!! So great!!!! 🎉🎉🎉🎉🎉

On Tue, Mar 19, 2019, 11:23 AM Sasha Boginsky notifications@github.com wrote:

@jywarren https://github.com/jywarren @ChrisLowe-Takor https://github.com/ChrisLowe-Takor I have been working in this library for a bit now and just wanted to give an update on this issue! I think the problem was because we were applying transformations on the images assuming they are squares, but really they don't have the same height and width (difference of around 150px). By setting edgeMinWidth and edgeMinHeight options equal to their w/h when instantiating them and adding some conditional logic in the scaling class, scaling no longer creates distortions:

[image: bug5] https://user-images.githubusercontent.com/41092741/54618212-06409d80-4a39-11e9-8684-cdcbfa1bdefa.gif

Please let me know if you have any thoughts on this!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/Leaflet.DistortableImage/issues/85#issuecomment-474420914, or mute the thread https://github.com/notifications/unsubscribe-auth/AABfJ9Wnwdgl7yl5vuBJJgf9m1go6YHSks5vYQDFgaJpZM4RlHSj .

sashadev-sky commented 5 years ago

@jywarren @ChrisLowe-Takor Ok take 2 -- this is now actually fixed! via #341 and #348. The solution was to switch latLngToLayerPoint to project() 🤯meanwhile we've all been rewriting matrix algorithms. I do see though, that when you move the handles slowly the image jitters a bit. a. luckily this is not the case for rotate so this narrows things down a bit.

Should I close this an open a new issue for that?