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

Share knowledge on maintaining SwiftUI frames #40

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

allenhumphreys
Copy link

These changes achieve 2 main things:

  1. The SwiftUI view's correctly follow the MKMapAnnotationView's frame around on the map. This fixes using onTapGesture directly on the SwiftUI view that represents the annotation
  2. The hosted SwiftUI is now updated (when Map is re-evaluated, which means when coordinateRegion changes, the annotations will be re-calculated. This allows hosting views that have inputs.

The result can be something like this:

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-03-11.at.11.55.48.mov
struct AnnotationPinView: View {
    let isSelected: Bool

    var body: some View {
        Image(systemName: "mappin")
            .renderingMode(.template)
            .aspectRatio(contentMode: .fit)
            .frame(width: 30, height: 30)
            .background(.green)
            .shadow(color: .black.opacity(0.5), radius: 0.9, x: 0.0, y: 2.0)
            .clipShape(Circle())
        // Note: Scale effect does not change intrinsicContentSize on the hosting view
            .scaleEffect(isSelected ? 1.5 : 1, anchor: .bottom)
            .animation(.interpolatingSpring(stiffness: 300, damping: 15, initialVelocity: 30), value: isSelected)
            .foregroundColor(isSelected ? .yellow : .blue)
    }
}
Map(
    coordinateRegion: $coordinateRegion,
    annotationItems: items,
    annotationContent: { itemModel in
        ViewMapAnnotation(
            coordinate: itemModel.coordinate,
            anchorPoint: CGPoint(x: 0.5, y: 1.0)
        ) {
            AnnotationPinView(isSelected: currentItem.id == itemModel.id)
                .onTapGesture {
                    currentItem = itemModel
                }
        }
    }
)
.ignoresSafeArea()

@iakov-kaiumov
Copy link

iakov-kaiumov commented Mar 12, 2023

Wow! Your solutions works like a charm!

  • I faced another issue with my implementation. My solution does not change the original MKAnnotationView frame. For some reason the original frame size is (10, 10). Hence, the problem arises when using clustering and annotations larger than (10, 10), because MKMapView uses frame size to determine whether annotations intersect or not. Your implementation works as expected.
  • You also somehow managed to solve MapAnnotations not centered properly #14
  • It would be really great if you added anchorPoint implementation for iOS lower than 16. If you do, I can close my PR.

if #available(iOS 16.0, *) {
anchorPoint = mapAnnotation.anchorPoint
} else {
// Fallback on earlier versions

Choose a reason for hiding this comment

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

Please, add fallback on earlier versions using centerOffset

if #available(iOS 16.0, *) {
anchorPoint = mapAnnotation.anchorPoint
} else {
// Fallback on earlier versions

Choose a reason for hiding this comment

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

Please, add fallback on earlier versions using centerOffset

@pauljohanneskraft
Copy link
Owner

pauljohanneskraft commented Mar 12, 2023

There are unfortunately a few issues that seem quite foundational to this solution. I would like to keep ViewMapAnnotation as a struct and would like to reduce the dependency between them (i.e. not have any references between them). With this implementation, we would lose both and have less type-safety and a less concise interface. Further, there seem to be a few UIKit "hacks" that might break in future or past OS versions. I will have a look in the next couple of days for a better solution.

@allenhumphreys
Copy link
Author

@pauljohanneskraft I get it, I just wanted to share so you could give the right solution more thought.

I don't expect this PR to be merged.

@pauljohanneskraft
Copy link
Owner

pauljohanneskraft commented Mar 12, 2023

I'm definitely grateful for your and @iakov-kaiumov 's support - I see this as one of the main issues of this library at the moment, but haven't yet found the time to really dive into this. It would further need to work for iOS 13+ until 16 and beyond, which is why I'm hesitant to put possibly breaking code into it. However, I can also understand that it is frustrating that the current implementation is also not working as expected.

@allenhumphreys
Copy link
Author

I was just perusing the API and found the thing I had been looking for. I think this will remove the need for the circular reference AND the need for class.

https://developer.apple.com/documentation/mapkit/mkmapview/1452512-view

@allenhumphreys
Copy link
Author

@pauljohanneskraft I've updated to use view(for:) introducing a protocol to help ensure the view even needs to be updated. This removes the need for the circular reference and the mutability of the annotation type. I'm unsure whether updating the annotation accounting mechanisms would make sense or be possible for the case of updates (as opposed to inserts/removals).

I don't really see a way to statically know the type of the annotation view in a way that doesn't require a conditional cast. But this approach eliminates the need for Any in the function signature.

@allenhumphreys
Copy link
Author

@iakov-kaiumov I will be checking on iOS 15 anchorPoint implementation tomorrow as that's an outstanding thing for me at work, too. If I get a good solution verified, I'll update this PR, but I think it'll look like your solution using the MKAnnotationView.centerOffset with some frame calculations. I just haven't had time to test it yet to prove that it actually works identically.

@allenhumphreys
Copy link
Author

@iakov-kaiumov I've updated to use centerOffset on pre-iOS 16 calculating the offset myself.

@iakov-kaiumov
Copy link

I'm facing some weird behaviour with your latest commit. Annotations change their locations when I move the map.

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-03-13.at.22.08.02.mp4

@allenhumphreys
Copy link
Author

@iakov-kaiumov That looks suspiciously like they're attempting to avoid the safe area. I'm not able to see that behavior on my little test rig though. You could try enabling the backgroundColor on the MKAnnotationView to see if the SwiftUI view contents are not aligned with the annotation view.

I'd need a reproduction case to help investigate it.

@iakov-kaiumov
Copy link

iakov-kaiumov commented Mar 14, 2023

@allenhumphreys
I found the cause of this issue. It happens when I embed Map into the NavigationView and use .edgesIgnoringSafeArea:

NavigationView {
    ContentView()
        .edgesIgnoringSafeArea(.all)
        .navigationTitle("Title")
}

@iakov-kaiumov
Copy link

@allenhumphreys
It is not a big problem in fact. Combining NavigationView and .edgesIgnoringSafeArea(.bottom) works fine.

@allenhumphreys
Copy link
Author

@iakov-kaiumov i wonder if the safe area disabling thing from #25 would address the problem in navigation view.

My primary use case disables safe area, so that's how I've been testing

@iakov-kaiumov
Copy link

@pauljohanneskraft
Do you have any progress with this issue?

@pauljohanneskraft
Copy link
Owner

Unfortunately no, I have drafted a new MapAnnotation setup which would allow the updating part, but nothing to present yet. It involved not creating the MKAnnotation objects as property of the MapAnnotation, but rather creating and updating it on demand and storing it inside Map.Coordinator as part of another dictionary... This will be quite some work, so it will most definitely take some time to implement - please use the workaround of replacing annotations for now (or adapt the library to your own preference on your own branches ofc)

Regarding the layout issue, I'm still confused why these complicated solutions are necessary, since they should not be so complicated, right? I will need to setup a UIKit-only project (and then introduce parts of this library) to check where the layout suddenly introduces these issues, since I cannot imagine that we would need to use dynamic objc features or delayed layouting... Is this a bug of UIHostingController or sth?

Unfortunately, I do not have time to investigate so deep into this at the moment, sorry 🙈 If you happen to find a simpler solution or can help with some investigation work, I will be happy to assist though

@allenhumphreys
Copy link
Author

@pauljohanneskraft I suspect the SwiftUI frame's frames not moving is an artifact of how MKMapView moves the annotation view's frame around. Again it's a suspicion, but I believe the hosting view relies on a regular update about its parent's frame in its parent's parent's coordinate system. I think MKMapView is possibly bending the usual rules of UIKit frame updates/layouts in a manner that breaks SwiftUI hosting views.

Inspecting the view hierarchy of MapKit's Map view actually reveals that SwiftUI has a custom subclass of MKMapView, likely hooking into private APIs to change the way the annotations get moved or otherwise provide signals to the SwiftUIAnnotationView that they have.

My changes are the result of a very very VERY intense investigation into making SwiftUI onTapGesture work correctly in a custom Map implementation.

This set of changes does 2 things.

  1. Forwards SwiftUI content updates to the MKAnnotationView's hosting controller. This change is independent of the frame fixing
  2. Fixes the SwiftUI view's frame.

They can really be taken as separate entities. And when boiled down to the minimum, fixing the frame is actually just a little bit of code.

It is worth noting, on the frame front. That MKAnnotationView's documentation explicitly says you MUST set the size of your MKAnnotationView or else it'll always be zero. Using constraints against the subview will do that, or setting bounds.size like I did will do that (also, fyi, don't set frame.size or else it updates weirdly).

@iakov-kaiumov
Copy link

@pauljohanneskraft
Okay, I think that I will close my pull request as I assume that @allenhumphreys's solution is better and has more chances to be merged.

@iakov-kaiumov
Copy link

@allenhumphreys
I've combined #25 and your branch together and now NavigationView + .edgesIgnoringSafeArea(.all) problem disappeared. So, I assume that you can safely merge #25 into your branch.

@vojtabohm
Copy link

Will this be merged? :)

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.

4 participants