search

Lickability / PinpointKit

Send better feedback
MIT License
1.13k stars 79 forks source link

Replace deprecated GLKView with MTKView #282

Closed woxtu closed 2 years ago

woxtu commented 3 years ago

Closes #268

What It Does

This PR replaces GLKView with MTKView to fix warnings related to GLKView:

How to Test

Add a blur annotation to your screenshot.

Notes

I added a workaround to resolve the issue of image flipping vertically when run in the simulator. See this article for reference.

mliberatore commented 3 years ago

Thanks, @woxtu!

I’m seeing a slight regression with the Metal implementation. The contents within the blurred rectangle are offset quite a bit from the area that the rectangle covers. The Before screenshot shows what’s on master, that the contents of the rectangle more closely match the positioning of the content beneath it.

Before

IMG_7347E3BC327A-1

After

File|

woxtu commented 3 years ago

@mliberatore Thank you for your testing. Unfortunately, I cannot reproduce the offset. Could you tell me about your environment? FYI, I tested in the simulator (iOS 14.2) and iPhone 8 (iOS 13.5.1) using Xcode 12.2.

mliberatore commented 3 years ago

Hi @woxtu. I’m running Xcode 12.2 (12B45b). I just ran a few more tests on different devices, and the offset is occurring for me on 2 of the 3 test iPhones that I have handy. It occurs on:

It does not occur on:

I also tried on the iPhone 12 Mini simulator this morning, and couldn’t reproduce it there. Between the devices that you and I have tried, my hunch is that this is only happening on screens with 3x scale, and only on hardware devices, not simulators.

Here’s a screenshot of it on the 6 Plus, where it appears as though the offset is even greater:

IMG_0C0C6D4849E5-1

Also, turning off the blur entirely just returning the image from BlurAnnotation.blurredImage makes it a bit easier to see the offset:

IMG_ED87BAC7700F-1

mliberatore commented 3 years ago

I think this might be the answer to the issue, as the problem seems to be similar: https://stackoverflow.com/a/56334696

It’d warrant more testing, but for example, we could call:

MTKView?.drawableSize = annotation?.image.extent.size ?? .zero

from annotation’s didSet, after MTKView?.layer.mask = layer.

mliberatore commented 3 years ago

Hi @woxtu! I just wanted to check in on this pull request. Please let us know if you’d like to make the changes discussed above, otherwise we’d be happy to make them on our end soon.

Kondamon commented 3 years ago

Hello @woxtu, could you resolve the offset or are there any updates related to this issue? Thank you!

jbouaziz commented 3 years ago

@woxtu would that PR add support for Mac Catalyst ? GLKit is not available but Metal Kit is.