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

Create a struct for ZoomLevel #95

Closed
Drabble opened this issue May 11, 2022 · 6 comments · Fixed by #118
Closed

Create a struct for ZoomLevel #95

Drabble opened this issue May 11, 2022 · 6 comments · Fixed by #118
Assignees
Labels
feature-request Request for a new feature good first issue Good for newcomers

Comments

@Drabble
Copy link
Collaborator

Drabble commented May 11, 2022

We probably should add a struct for ZoomLevel.

Originally posted by @maxammann in #87 (comment)

🤔 Expected Behavior

Zoom is a f64 which represents the current zoom on the map. It needs to be a floating number because we want to be able to zoom smoothly on the map.

ZoomLevel is a u8 which represents the level of a tile within the map's quad tree. ZoomLevel 0 would be one tile that contains the entire map, ZoomLevel 1 would be a part of the map if it was split in 4, etc.

We can derive the current ZoomLevel from the Zoom using the constant ZOOM_BOUNDS defined in the coords.rs file.

😯 Current Behavior

Currently ZoomLevel is always stored as a u8 directly.

💁 Possible Solution

We should create a custom type or a struct to store the zoom level.

🔦 Context

It would make the code easier to read.

💻 Examples

The function below could be changed from:

pub fn scale_to_zoom_level(&self, z: u8) -> f64

to:

pub fn scale_to_zoom_level(&self, z: ZoomLevel) -> f64
@maxammann maxammann added good first issue Good for newcomers feature-request Request for a new feature labels May 11, 2022
@ybiletskyi
Copy link
Contributor

I'll try to pick up this issue. Can any one assign it to me?

@maxammann
Copy link
Collaborator

Sure! If you have any questions feel free to contact me on matrix

@nyurik
Copy link
Member

nyurik commented May 25, 2022

should this be an enum with either z8 or f32 values? This way we can work with it transparently, while still enjoying the u8 perf optimizations... Although this could be a case of premature optimization, and instead it should just be two values -- TileZoom(u8) and ViewportZoom(f32), and each function will be clear which one it is using.

@maxammann
Copy link
Collaborator

I think both "zoomes" are conceptionally quite different and their documentation will also differ. While zoom is a property of the "viewport", the zoom level is a property of a tile (like you said).
Accepting a f32 for a function which expects the zoom level of the currently displayed layer out of tiles probably does not make sense.

@ybiletskyi
Copy link
Contributor

Should this be a type alias instead of creating a new struct? If it's just need for improving code reading.

@maxammann
Copy link
Collaborator

I think a new struct makes sense, so we can actually define functions on a ZoomLevel and make sure people understand what this u8 means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for a new feature good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants