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

Position puck in lower portion of screen. #92

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Position puck in lower portion of screen. #92

merged 1 commit into from
Aug 14, 2024

Conversation

michaelkirk
Copy link
Collaborator

Description

Fixes #80, as a partial alternative to #82.

@Patrick-Kladek - if I've missed something necessary for lowering the puck, please let me know. If the other changes in #82 are fixing other things, please re-scope #82 or follow up in a separate PR explaining the changes.

Before

Puck is centered.

puck-before-lowering

After

Puck is lower.

puck-lower-0 4 Screenshot 2024-08-07 at 14 46 50

iPhone SE

Screenshot 2024-08-07 at 14 47 37 Screenshot 2024-08-07 at 14 47 30

Copy link
Collaborator

@hactar hactar left a comment

Choose a reason for hiding this comment

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

Tested this via the example app, including iPad, and it works.

With #82 including other improvements that aren't fully documented, lets merge this partial fix first and evaluate the other improvements separately.

@hactar hactar merged commit b8bdcfd into main Aug 14, 2024
2 checks passed
@hactar hactar deleted the mkirk/lower-puck branch August 14, 2024 10:53
@amjaliks
Copy link

This isn't the best solution. It assumes everyone will be fine with this slightly lower puck location. And it completely ignores userAnchorPoint.
While the solution implemented by @Patrick-Kladek in #82 takes in account userAnchorPoint.

@michaelkirk
Copy link
Collaborator Author

The genesis of #80, was that, while on a monthly technical steering call, we realized that almost everyone on the call had implemented their own hack to get something similar to this behavior. We decided on that call that this was strong evidence for making "lower third" a better default than "centered".

As before, see NavigationMapViewCourseTrackingDelegate.updateCamera(_:location:,routeProgress:) if you want to customize this behavior outside of the default.

it completely ignores userAnchorPoint.

My understanding is that userAnchorPoint tells you where the user course view is, relative to the map view, for the purpose of applying gestures driven transformations like zooming. It's only a getter.

It sounds like you are saying that, instead of a getter, you'd like to be able to set userAnchorPoint to position the user puck view. Is that right? If not, can you try giving a more specific example of the behavior you're looking for.

@hactar
Copy link
Collaborator

hactar commented Aug 14, 2024

This isn't the best solution. It assumes everyone will be fine with this slightly lower puck location. And it completely ignores userAnchorPoint. While the solution implemented by @Patrick-Kladek in #82 takes in account userAnchorPoint.

@Patrick-Kladek solution is not closed, this is just one part of it that was factored out as it works and is easy to understand. If you'd like to help getting #82 merged then you can help by answering the discussion questions there and reviewing the PR. ☺️

@Patrick-Kladek
Copy link
Contributor

Hi everyone, I'm back from my holiday so please excuse the late reply.

@michaelkirk Customizing the camera via the delegate wasn't working properly for me. With the default camera & puck implementation, we noticed that the actual user position was offset from the puck, you could notice this when making a 90° turn.

@amjaliks In #82 I fixed both issues (puck offset & lower puck on screen) by making the userAnchorPoint the single source of truth. Looking at the changes in this PR, it doesn't solve the puck offset from the actual user location.

Another thing I've noticed is you've chosen 40% of the screen height for the puck position which I think is too high. In my PR I choose a different value, which might also not be perfect for every use case, so I think we should make this customizable, either via a percentage or via EdgeInsets.

@michaelkirk
Copy link
Collaborator Author

Another thing I've noticed is you've chosen 40% of the screen height for the puck position which I think is too high

You're right. I think the default should be lower. That's an easy change to make, but I think your other points might be trickier.

Firstly, I've opened #94 to discuss the "bug fix" parts of puck placement vs map camera.

Customizing the camera via the delegate wasn't working properly for me.

As for customizing, that's a good point. I don't think it's actually currently possible. I think we'd need to plumb NavigationMapViewCourseTrackingDelegate.updateCamera to an outer delegate that can actually be manipulated to override.

Specifically, navigationMapView would have to ask courseTrackingDelegate, which is RouteMapViewController, which would have to ask NavigationViewController, which could (finally!) ask the user code at NavigationViewControllerDelegate.

Which is annoying, but unless I missed something, I think that this is exactly the same for the userAnchorPoint approach in #82, right?

Specifically, navigationMapView would have to ask navigationMapDelegate, which is RouteMapViewController, which would have to ask NavigationViewController, which could (finally!) ask the user code at NavigationViewControllerDelegate.

As an aside, it's a little awkward that NavigationMapView has two different ways to position the camera - one with userAnchorPoint and one with updateCamera. I'm not sure if it's coherent to have both. We didn't create that problem, but we inherited it. I'm not really sure how / if we should resolve that.

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.

Navigation Puck is centered, but should be in bottom third so you can see farther ahead.
4 participants