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

Improving Camera Conversion #54

Merged

Conversation

hactar
Copy link
Collaborator

@hactar hactar commented Oct 6, 2024

Issue/Motivation

I noticed that in certain situations, even though my camera was setup as pitchRange = .free, it kept switching to .freeWithinRange(0, 59.9999999). When back converting the MLNMapView camera we were missing determining the pitch range based on our enum: the camera would always be set to .freeWithinRange instead of trying to go for .free or .fixed too if applicable.

Tasklist

  • [N/A] Include tests (if applicable) and examples (new or edits)
  • [N/A] If there are any visual changes as a result, include before/after screenshots and/or videos
  • [N/A] Add #fixes with the issue number that this PR addresses
  • [N/A] Update any documentation for affected APIs
  • [N/A] Update the CHANGELOG

@hactar
Copy link
Collaborator Author

hactar commented Oct 6, 2024

/Users/runner/work/swiftui-dsl/swiftui-dsl/Sources/MapLibreSwiftUI/MapViewCoordinator.swift:336:1: error: (andOperator) Prefer comma over && in if, guard or while conditions.

Really, thats a thing now? 😅 Not a fan, && is clearly readable for me, "," is not 🤷‍♂️.

@ianthetechie
Copy link
Collaborator

Really, thats a thing now? 😅 Not a fan, && is clearly readable for me, "," is not 🤷‍♂️.

wat... Ok, I think I'd use this style specifically if I have a multi-condition if statement that also includes let bindings, because that's how the syntax works for those. But otherwise yeah this is less clear for me too. I'll disable that check.

ianthetechie added a commit to michaelkirk/swiftui-dsl that referenced this pull request Oct 7, 2024
@ianthetechie ianthetechie added the norelease Don't create an automatic release label Oct 7, 2024
@ianthetechie ianthetechie merged commit 9b306fc into maplibre:main Oct 7, 2024
1 of 2 checks passed
ianthetechie added a commit that referenced this pull request Oct 7, 2024
* Fix: recenter doesn't immediately take effect

Note: I only ever reproduced the bug in the context my app, which uses
this library via Ferrostar, while using the CoreLocationProvider, not
the SimulatedLocationProvider. But I expect this to be an issue for
anyone using this API.

To reproduce the bug in my app:

Setup: (all of this is working more or less as expected):

- While stationary (not sure if this is required)
- start a route
- see the user location puck and map centered on my location on the
  ground.
- pan the map off the route
- see the user location puck disappear, but you still see the smaller location
  indicator on the map. (as expected)
- see the "recenter me" button appear
- tap the "recenter me" button

Now here's the problem:

At this point I'm expecting the map to recenter with the user location
puck on my location on the ground.

But instead, I see the user location puck centered at the *current* map
position - where I'd previously panned to, *not* at my location on the ground.

Interestingly, if you'd repeat the process at this point - panning and
then re-tapping the "center me" button, it would *work* this second time.

---

The new behavior is definitely an improvement. However, if you've zoomed
way out while exploring the map away from your route, there is a
slightly noticeable two-part action while we wait for the new completion
block - first re-centering and then zooming. It's unquestionably better
than the current behavior though.

* Fixes from @hactar's branch

* Disable swiftformat andOperator (see #54)

* Fix inadvertent regression; update CHANGELOG

---------

Co-authored-by: Ian Wagner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
norelease Don't create an automatic release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants