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

Some handlers not working ... after fix #517 #1007

Closed
themacboy opened this issue Aug 4, 2022 · 23 comments · Fixed by #1033
Closed

Some handlers not working ... after fix #517 #1007

themacboy opened this issue Aug 4, 2022 · 23 comments · Fixed by #1033
Labels

Comments

@themacboy
Copy link
Contributor

Good morning,

for some reason drag, rotate, freerotate and other handlers won't do his job in some circumstances.

After a lot of time debugging the code, to find the problem I have noticed that changes in #517 has broke the usual behavior of this handlers.

I have noticed that the problem is in the function:

_cornerExceedsMapLats: function(zoom, corner, map) {...} on file DistortabeImageOverlay.js

and the problem becomes in the next check:

    if (zoom === 0) {
      exceedsTop = map.project(corner).y < 2;
      exceedsBottom = map.project(corner).y >= 255;
    } else {
      exceedsTop = map.project(corner).y / zoom < 2;
      exceedsBottom = map.project(corner).y / Math.pow(2, zoom) >= 255;
    }

for some reason, in the map I use, the variable map.project(corner).y returns a negative value, and obviously this is always lower than 2

then next both checks return True:

exceedsTop = map.project(corner).y < 2;
or
exceedsTop = map.project(corner).y / zoom < 2;

and it make leave the normal execution of the handler function.

Im still not use why map.project(corner).y return a negative value, probably due the system coordinates Im using.

And probably this is a bug on the logics in the code. @jywarren

@themacboy
Copy link
Contributor Author

console print of the check and value of variable :

exceedsTop = map.project(corner).y / zoom < 2;

image

@themacboy
Copy link
Contributor Author

themacboy commented Aug 4, 2022

Example of other output values:

map.project(corner).x = 12888737.999999993
map.project(corner).y = -147853214.99999997
object corner = {lat: 41.72958170926682, lng: 1.8309573804288197}

@jywarren
Copy link
Member

Hmm, thanks for documenting this... can you share a screen recording of the issue? What projection are you using, L.CRS.Simple? Is there any way we can try out the code you're using interactively? Thanks again!

@themacboy
Copy link
Contributor Author

Sorry Im not skilled on all this, not sure what kind of L.CRS im using, i copy you my lines to creqate the map:

        this.ocrs = new L.Proj.CRS('EPSG:25831', '+proj=utm +zone=31 +ellps=GRS80 +units=m +no_def', {
            resolutions: this.resolutions,
            origin: [0, 0]
        });

@themacboy
Copy link
Contributor Author

themacboy commented Aug 19, 2022

this is the code I have modified to "patch" the issue (but really is a worng solution):

    var exceedsTop;
    var exceedsBottom;
    if (zoom === 0) {
      exceedsTop = Math.abs( map.project(corner).y ) < 2;
      exceedsBottom = Math.abs( map.project(corner).y ) >= 255;
    } else {
      exceedsTop = Math.abs( map.project(corner).y / zoom ) < 2;
      exceedsBottom = Math.abs( map.project(corner).y / Math.pow(2, zoom) ) >= 255;
    }
    return (exceedsTop || exceedsBottom);
  },

AS you see I simply set a Math.abs fucntion to move all values to positive and avoid the incorrect boolean check that broke the handlers.

But as said, that not correct too, it is simply a logical patch. As you see in my previous message the number returned in

map.project(corner).y

they are totally crazy on the CRS map, not sure why.. I don't have the necessary knowledge in all of this.

When at work I will try to record a video for you.

@jywarren
Copy link
Member

jywarren commented Aug 19, 2022 via email

@jywarren
Copy link
Member

Is it possible the projection is uniquely rejecting out of bounds coordinates and that we could bypass by actually limiting them to just under 180 or 90, so like 179.999 and 89.999? Just to avoid the situation entirely.

@jywarren
Copy link
Member

Or might another solution be to just turn off this bounds limiting check for the L.CRS.simple projection until we can otherwise resolve it?

@jywarren
Copy link
Member

@steveruiz @jbogdani does this last suggestion seem an ok workaround for you?

@steveruiz
Copy link

Yes - I think entirely turning off the bounds limiting check for L.CRS.Simple is the right way to go.

@jywarren
Copy link
Member

Excellent, i will craft a first-timers-only issue from it and it should get completed soon then. Thank you!

@jywarren
Copy link
Member

Just copying in info here as i go:

_cornerExceedsMapLats: function(zoom, corner, map) {
var exceedsTop;
var exceedsBottom;
if (zoom === 0) {
exceedsTop = map.project(corner).y < 2;
exceedsBottom = map.project(corner).y >= 255;
} else {
exceedsTop = map.project(corner).y / zoom < 2;
exceedsBottom = map.project(corner).y / Math.pow(2, zoom) >= 255;
}
return (exceedsTop || exceedsBottom);
},
setCorners: function(latlngObj) {
var map = this._map;
var zoom = map.getZoom();
var edit = this.editing;
var i = 0;
// this is to fix https://github.com/publiclab/Leaflet.DistortableImage/issues/402
for (var k in latlngObj) {
if (this._cornerExceedsMapLats(zoom, latlngObj[k], map)) {
// calling reset / update w/ the same corners bc it prevents a marker flicker for rotate
this.setBounds(L.latLngBounds(this.getCorners()));
this.fire('update');
return;
}
}

@jywarren
Copy link
Member

This method is called twice, both times in that file:

// this is to fix https://github.com/publiclab/Leaflet.DistortableImage/issues/402
for (var k in latlngObj) {
if (this._cornerExceedsMapLats(zoom, latlngObj[k], map)) {

and

if (this._cornerExceedsMapLats(zoom, corner, map)) {

@jywarren
Copy link
Member

OK, I've created an issue for it here: #1032 and asked a first-timer to try solving it. Thanks!

@jywarren
Copy link
Member

Hi all, once the final steps are made in #1033 would you mind testing them out? Thank you!!

@jywarren
Copy link
Member

jywarren commented Oct 1, 2022

Hello @themacboy @steveruiz @jbogdani @tadiraman can you check out the release candidate at #1036 (v0.21.9) to confirm if it does or doesn't address the issues you've been having? (remembering that we are guessing they are all inter-related but it's still possible this may fix things for some but not all of you?)

Thank you!

@jywarren
Copy link
Member

jywarren commented Oct 9, 2022

Hi @themacboy @steveruiz @jbogdani @tadiraman just checking if you've been able to give this a try. No rush though. Thanks! <3

@themacboy
Copy link
Contributor Author

With the exception in last update it is working as normal for me.

Not sure if there can be other issues, but now is working for sure

@jywarren
Copy link
Member

Excellent. I'm going to go ahead and make the release in the next day or two i hope. Will be great to hear from others, but if it fixes your issue, it's enough to move forward with a release. We can distinguish which other issues were or weren't resolved afterwards if need be.

Thanks!

@jywarren
Copy link
Member

Published!!!

@themacboy
Copy link
Contributor Author

themacboy commented Oct 13, 2022

opsss but news! there is an error yet.
Sorry when I test the changes something remain in my browser cache.

Now I go to PR a solution #1141

@jywarren
Copy link
Member

jywarren commented Oct 13, 2022 via email

@jywarren
Copy link
Member

Got your fix in #1141 and merged it. Should be able to publish a new version again soon. Thanks!

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

Successfully merging a pull request may close this issue.

3 participants