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

[v2.2.0] Add Rotation Capability To OverlayImage #1315

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

Robbendebiene
Copy link
Contributor

Currently the OverlayImage layer only accepts a rectengular bounding box based on two corner points.
This doesn't allow rotating overlaid images like it is possible in other map frameworks.

Mapbox for example requires a latitude/longitude for each corner of the image.

For Leaflet the following plugin/layer exists which just requires 3 points to be defined (because it derives the 4th point itself): https://github.com/IvanSanchez/Leaflet.ImageOverlay.Rotated

This PR is based on the beforehand mentioned Leaflet.ImageOverlay.Rotated layer and adds a third point to rotate/skew the image.

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Hi, and thanks for your contribution!

Whilst testing, it seems to work well, except I'm confused about which points are transformed. For example, see this image, when there is no rotationPoint defined:
image

As far as I understand, the red point should be the top-left, and the blue should be in the bottom-right? When I add the rotationPoint, it looks correct to me:

image

Can you just confirm the behaviour a bit more? Maybe the point above is the intended functioning, but I'm not sure.

@JaffaKetchup JaffaKetchup changed the title Add rotation capability to OverlayImage layer [v2.1.0] Add Rotation Capability To OverlayImage Jul 21, 2022
@Robbendebiene
Copy link
Contributor Author

Thanks for the fast reply.
You are right, I must have messed something up, in theory all markers should be located on the corners of the image.

I'm looking into this.

@JaffaKetchup
Copy link
Member

No problem. Just note that it wasn't the bottom image that was the issue to me (you can't stretch the image, and it looks like the vertical offset from corner to marker averages to 0 on both left and right, which is correct), it was that the top image should have the markers swapped vertically on the corners, if you get what I mean.

@Robbendebiene
Copy link
Contributor Author

No problem. Just note that it wasn't the bottom image that was the issue to me (you can't stretch the image, and it looks like the vertical offset from corner to marker averages to 0 on both left and right, which is correct),

Actually I can stretch/skew the image so all 3 points are aligned correctly. It worked, but when cleaning up the code I must have changed something..

This is how it is supposed to work: http://ivansanchez.github.io/Leaflet.ImageOverlay.Rotated/demo.html

it was that the top image should have the markers swapped vertically on the corners, if you get what I mean.

I basically haven't touched the code that handles the positioning of the image when there is no rotationPoint given.

@JaffaKetchup
Copy link
Member

Actually I can stretch/skew the image so all 3 points are aligned correctly. It worked, but when cleaning up the code I must have changed something.

I'm not 100% sure I like this (the way the demo works). I think we should have a parameter that toggles skewing, where disabled is like the way it works currently, and enabled is the way the demo site works.

I basically haven't touched the code that handles the positioning of the image when there is no rotationPoint given.

Hmm, that's quite weird then. Maybe the example coords are just specified wrong. I wouldn't worry about it in this case.

@JaffaKetchup JaffaKetchup self-assigned this Jul 21, 2022
@ibrierley
Copy link
Contributor

Just reading up on this...looks ok when I test, but haven't had chance to dig much...what's with the misaligned marker points above ?

@JaffaKetchup
Copy link
Member

@Robbendebiene As v2.1.0 is now ready to merge and release, I'll have to leave this PR for the next release.

@JaffaKetchup JaffaKetchup changed the title [v2.1.0] Add Rotation Capability To OverlayImage Add Rotation Capability To OverlayImage Jul 22, 2022
This is done to solve a problem for the rotated image caused by the LatLngBounds class which may swap corner point lat/lngs.
@Robbendebiene
Copy link
Contributor Author

Robbendebiene commented Jul 25, 2022

I've finally figured out why sometimes the corner points of the image correctly align with the markers/provided lat lng points and sometimes not.
It's because the LatLngBounds class creates its own "top left" and "bottom right" points based on the passed corner points. If the provided points are identical to those calculated by the bounding box everything fits, but when they differ (because the provided corner points aren't northWest and southEast points) the image corners and markers will be misaligned. So I cannot use the LatLngBounds because it doesn't always preserve the original lat/lng points.

I decided not to create an extra layer and instead created another RotatedOverlayImage class that can be passed to the OverlayImageLayer. Hope that this is okay.

@JaffaKetchup
Copy link
Member

Ah I see. Can't test right now, but should be able to soon. Maybe creating a separate class is the best option anyway, good choice!

@JaffaKetchup JaffaKetchup changed the title Add Rotation Capability To OverlayImage [v2.2.0] Add Rotation Capability To OverlayImage Jul 26, 2022
@JaffaKetchup JaffaKetchup removed their assignment Jul 26, 2022
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

LGTM, seems good to me.

@JaffaKetchup
Copy link
Member

Thanks for your contributions!

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.

3 participants