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

Don't offset views rendered on top of snapshots #733

Merged
merged 3 commits into from
Feb 1, 2023

Conversation

fynngodau
Copy link
Collaborator

This PR fixes view rendered on top of the map during snapshots being offset incorrectly.

For no discernible reason, in mapbox/mapbox-gl-native#9179 (comment), it was introduced that a view that is a child of the map view is rendered 10px to the right, 10px to the bottom of its original position.

This introduces visual glitches already visible in the screenshot linked above. Therefore, this PR removes this offset.

It also fixes any other child views a user may choose to put on a map (the use case I have in mind is custom info windows).

In the following screenshots, bottom is live map, top is snapshot – it is the "Snapshot Activity" in the sample app:

Before After
Screenshot_1674726209 Screenshot_1674726117

@ovivoda
Copy link
Contributor

ovivoda commented Jan 28, 2023

@fynngodau for the same purpose you could also call

public static Bitmap mergeBitmap(@NonNull Bitmap background, @NonNull Bitmap foreground, float left, float top) just use 0f and 0f for left and top

I do not think this change is really necessary.

@fynngodau
Copy link
Collaborator Author

@ovivoda It is true we could also change the place in the maplibre code that calls mergeBitmap(Bitmap, Bitmap) to instead call mergeBitmap(Bitmap, Bitmap, float, float) directly. However I have chosen to change this function instead because rendering a snapshot appears to be the only context in which it is called, and 10px times 10px offset really isn't a sane default here anyway.

@ovivoda
Copy link
Contributor

ovivoda commented Jan 29, 2023

@fynngodau my point here is backwards compatibility. If someone uses the old mergeBitmap method and does some maths based on that, the new behaviour will mess it up. This is why, I think that, if there is a method to achieve same thing, we should not change the old behaviour.

@fynngodau
Copy link
Collaborator Author

@ovivoda I see. Yes, considering that this API is public, it does consist a breaking change in some sense, though I doubt that any user of the API actively wants exactly a 10x10 shift of the image and therefore I would still prefer to change the default here, and announce the breaking change in a changelog.

I'll push a commit that leaves the method intact but changes the documentation so that users are aware of this behavior.

@ovivoda
Copy link
Contributor

ovivoda commented Jan 31, 2023

@fynngodau alright. If you think that this is really a must let's do it like this please:

  • mark current method as deprecated
  • create a new method to merge bitmap with no shifts

Then we have the best of both worlds and after some time/versions we can actually remove the old method.

But removing it now, or change the behaviour, I do not think it is expected in a library.

@fynngodau
Copy link
Collaborator Author

@ovivoda Seems like a good idea. What would be a good method name for such a new method?

@ovivoda
Copy link
Contributor

ovivoda commented Jan 31, 2023

this is a tough one @fynngodau

maybe mergeBitmaps will work

@fynngodau
Copy link
Collaborator Author

👍, I've added a new commit for this.

Copy link
Contributor

@ovivoda ovivoda left a comment

Choose a reason for hiding this comment

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

it looks good! Thanks @fynngodau

@ovivoda ovivoda merged commit 1d30ba7 into maplibre:main Feb 1, 2023
@fynngodau fynngodau deleted the fix-snapshot-views branch February 1, 2023 08:25
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 this pull request may close these issues.

3 participants