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

feat: Add exponential interpolation map zooming [skip ci] #2373

Merged
merged 26 commits into from
Apr 23, 2024
Merged

Conversation

kristofbolyai
Copy link
Member

@kristofbolyai kristofbolyai commented Mar 16, 2024

I wonder which PR's perquisite is this change.

Touchpad demo:

Screen.Recording.2024-03-17.at.11.41.44.mov

@kristofbolyai
Copy link
Member Author

With this PR done, @magicus, how do you think we should account for zooming in MapData?

We have two options:

  • Still go by the old zoom levels, which are percentage based, 0.3 means the map is rendered with a 30% scale.
  • Go by the new step system, which allows defining levels on an unified integer base.

@kristofbolyai kristofbolyai changed the title feat: Add exponential interpolation (log scale) map zooming feat: Add exponential interpolation map zooming Mar 17, 2024
@kristofbolyai kristofbolyai changed the title feat: Add exponential interpolation map zooming feat: Add exponential interpolation map zooming [skip ci] Mar 17, 2024
@kristofbolyai
Copy link
Member Author

@magicus Any chance you can find time to get this reviewed?

@magicus
Copy link
Member

magicus commented Apr 22, 2024

Sorry for the slow input on this.

I tested this branch, and discovered that the zoom of the map is dependent on the GUI scale. This means that on GUI scale 1, when max zoomed out, the entire continent is just like 25% of the visible space which is way too much zoomed out, and contrary, it is hard to zoom in enough to see all details.

And conversely, at GUI scale 5, when max zoomed out, you can hardly see the entire Province of Wynn, and when zooming in you get to absurd levels where you see like 20 blocks across.

I think that needs to be fixed as part of this PR, so there is a clear understanding what zoom level 1 and zoom level 100 means, e.g. on zoom level 1 you should see like (almost) the entire continent, and on zoom level 100 you should see a quarter of Detlas.

@magicus
Copy link
Member

magicus commented Apr 22, 2024

I'm also not happy with the naming. I think the new 1-100 scale should be what we use everywhere except when finally rendering, so it should just be called "zoom" (not anything with "steps"). To get a value we can send into the rendering engine, we can use something like "zoom factor". But right now, I think we are calculating this in multiple steps, where at some point we invert this using 1/zoom etc. I would like to see us send the "zoom" (1-100) value as far down as possible in the call stack, until we actually need to convert this into something that makes sense for the rendering.

edit: We could also call it "zoom level"; I realize that is how I think of it.

@magicus
Copy link
Member

magicus commented Apr 22, 2024

On the positive side, however, is that the zooming feels very good. It has a natural feel, and is just perfectly tuned to be fast enough while still being precise enough.

I tried looking for how the GUI scale comes into play, but did not figure it out. In the worst case, we can't do anything about it (which would be a shame); if so we might have to set our expectations based on a supposed commonly used value (2-3?).

@magicus
Copy link
Member

magicus commented Apr 22, 2024

I added functionality to show the current zoom level when pressing shift.

@ShadowCat117
Copy link
Contributor

I added functionality to show the current zoom level when pressing shift.

Doesn't have to be done here, but I'd love to see a slider for zoom level in the future akin to this
bbzUc

@magicus
Copy link
Member

magicus commented Apr 22, 2024

Agree. My hack of showing the zoom level as an overlay is kind of a step on the way. Presenting it as a slider instead of a number is clearer, and it will also allow direct manipulation. It's just much more work to implement a slider widget in Minecraft. If you feel up to it, please go ahead! You can create a new branch based on this, so you can use the new 1-100 zoom levels.

@ShadowCat117
Copy link
Contributor

Might save it for when the map screen gets redone. I've seen a few suggestions and have some ideas of my own for improvements but after the filter screen I'm taking a small break from ui work to do some of the fuy.gg features

@kristofbolyai
Copy link
Member Author

@magicus

46f333e fixes the zoom level not being stable for different GUI scales.

Below is demonstrated how the map basically fits the same way for different scales. POIs and text are unaffected.

1 GUI Scale:
2024-04-22_14 30 53

2 GUI Scale:
2024-04-22_14 31 07

5 GUI Scale:
2024-04-22_14 31 21

@kristofbolyai
Copy link
Member Author

@magicus

dc8fbd0 renames zoom steps to zoom levels. While I understand your intent of only having zoomLevel in our code, it's practically impossible, as we have 58 usages for zoomRenderScale just in AbstractMapScreen.

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

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

I've made a bunch of changes. Now I am happy with this PR. Are you okay with it?

@magicus
Copy link
Member

magicus commented Apr 23, 2024

we happen to support touchpad-style pinch-to-zoom by concept. :)

Do you know what would be required to add this in reality too? I guess it needs some digging into GLFW...

@magicus
Copy link
Member

magicus commented Apr 23, 2024

It seems to be stuck here glfw/glfw#90 (since 2013), with a solution here: glfw/glfw#2419 that has not been merged since 2023. :-(

@kristofbolyai
Copy link
Member Author

we happen to support touchpad-style pinch-to-zoom by concept. :)

Do you know what would be required to add this in reality too? I guess it needs some digging into GLFW...

I think we talk about a different thing. I mainly tested Mac's double finger scrolling. Not sure how to support real pinching (I forgot how pinching was supposed to work..)

@magicus
Copy link
Member

magicus commented Apr 23, 2024

Ah, I see. Yes, the double-finger scroll works fine. And I guess we'll have to wait another decade or so for pinch-zooming to be implemented in GLFW :-).

@magicus
Copy link
Member

magicus commented Apr 23, 2024

@kristofbolyai To be clear, I am awaiting a review from you as well, since I changed so much in the end.

Copy link
Member Author

@kristofbolyai kristofbolyai left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@magicus magicus added this pull request to the merge queue Apr 23, 2024
Merged via the queue into main with commit 54bc3af Apr 23, 2024
1 check passed
@magicus magicus deleted the map-zoom branch April 23, 2024 13:36
kristofbolyai added a commit that referenced this pull request May 13, 2024
* feat: Increase maximum zoom limit to 5x

* feat: Add exponential interpolation (log scale) map zooming

* feat: Also add the new zooming to the minimap

* refactor: Allow a bit more steps, add a constant default

* feat: Second iteration of map zooming logic

* feat: Allow float-steps

* fix: make mouse-scrolling a bit more sensitive

* fix: fix MinimapOverlay onConfigUpdate logic

* chore: remove outdated comment

* fix: fix CustomSeaskipperScreen zooming

* fix: add separate setZoomStep function

* feat: Add rendering of zoom level when pressing shift

* feat: Make the zoom ratio constant for every GUI scale

* chore: Rename zoom steps to zoom level

* chore: Change "zoom step" to "zoom level" in several places

* ci: spotless formatting

* chore: Fix scaleSteps as well

* fix: Clamp minimap zoom level for key bindings

* chore: Small refactorings

* ci: spotless formatting

* fix: Improve default zoom level and centering

* chore: Reorder for consistency

---------

Co-authored-by: Magnus Ihse Bursie <mag@icus.se>
Co-authored-by: magicus <magicus@users.noreply.github.com>
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.

3 participants