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.

Sign up for GitHub

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

Jump to bottom

fix: make horizontal repetition CRS dependent #1978

Merged
merged 7 commits into from
Nov 17, 2024
Merged

fix: make horizontal repetition CRS dependent #1978

merged 7 commits into from
Nov 17, 2024

Conversation

monsieurtanuki
Copy link
Contributor

What

Files

New file:

  • epsg3006_crs.dart: new example around EPSG3006, needed for tests

Impacted files:

  • camera.dart: special case around replicatesWorldLongitude
  • crs.dart: new bool getter replicatesWorldLongitude
  • main.dart: added EPSG3006 example
  • map_interactive_viewer.dart: special case around replicatesWorldLongitude
  • menu_drawer.dart: added EPSG3006 example
  • tile_coordinates.dart: moved code to new class TileCoordinatesSimplifier
  • tile_image_manager.dart: now using a TileCoordinatesSimplifier for more flexibility
  • tile_image_view.dart: now using a TileCoordinatesSimplifier for more flexibility
  • tile_layer.dart: special case around replicatesWorldLongitude
  • tile_range.dart: special case around replicatesWorldLongitude

@monsieurtanuki
fix: now works for crs with world replication or not
c06b370 
New file:
* `epsg3006_crs.dart`: new example around EPSG3006, needed for tests

Impacted files:
* `camera.dart`: special case around `replicatesWorldLongitude`
* `crs.dart`: new `bool` getter `replicatesWorldLongitude`
* `main.dart`: added EPSG3006 example
* `map_interactive_viewer.dart`: special case around `replicatesWorldLongitude`
* `menu_drawer.dart`: added EPSG3006 example
* `tile_coordinates.dart`: moved code to new class `TileCoordinatesSimplifier`
* `tile_image_manager.dart`: now using a `TileCoordinatesSimplifier` for more flexibility
* `tile_image_view.dart`: now using a `TileCoordinatesSimplifier` for more flexibility
* `tile_layer.dart`: special case around `replicatesWorldLongitude`
* `tile_range.dart`: special case around `replicatesWorldLongitude`
@monsieurtanuki
typo fix
1ec9e71
@monsieurtanuki monsieurtanuki mentioned this pull request Oct 16, 2024
Closed
@arneke
Copy link

arneke commented Oct 17, 2024

This branch works well in my application.

Still think the 1 << z is a bit problematic. Does https://demo.fleaflet.dev/crs_epsg4326 work for you locally ? (that is a grid with two tiles at z = 0)

@monsieurtanuki
Copy link
Contributor Author

This branch works well in my application.

Cool!

Still think the 1 << z is a bit problematic. Does https://demo.fleaflet.dev/crs_epsg4326 work for you locally ? (that is a grid with two tiles at z = 0)

@arneke You're right: I've just fixed that. Thank you for your meticulous review!

@JaffaKetchup JaffaKetchup requested review from mootw, JaffaKetchup and josxha and removed request for mootw and JaffaKetchup October 21, 2024 15:08
JaffaKetchup
JaffaKetchup requested changes Oct 21, 2024
View reviewed changes
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait, had a busy week!

lib/src/layer/tile_layer/tile_coordinates.dart Outdated Show resolved Hide resolved
@JaffaKetchup JaffaKetchup changed the title fix: now works for crs with world replication or not fix: make horizontal repetition CRS dependent Oct 21, 2024
@JaffaKetchup JaffaKetchup linked an issue Oct 21, 2024 that may be closed by this pull request
[BUG] unbounded horizontal scroll breaks most tile grids #1976
Closed
JaffaKetchup
JaffaKetchup approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

(Sorry for the wait, I've had a hugely busy few weeks!)

I think I'm OK with this, but would appreciate a review from someone else!

@monsieurtanuki
Copy link
Contributor Author

Thank you @JaffaKetchup for your review!
I'm open to new reviews and new suggestions.

josxha
josxha reviewed Nov 16, 2024
View reviewed changes
Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

I have the feeling that the fling animation has stutters when using a map with longitude repetition. Most notifable when crossing the -180/180 longitude. Could anyone check if it happens to them as well?

example/lib/pages/epsg3006_crs.dart Outdated Show resolved Hide resolved
lib/src/layer/tile_layer/tile_coordinates.dart Outdated Show resolved Hide resolved
@monsieurtanuki
renamed as "resolver" and removed "immutable"
db5511e 
Impacted files:
* tile_coordinates.dart: renamed as TileCoordinatesResolver and made const
* tile_image_manager.dart: removed the immutable metatag; used a setter and refactored
* tile_image_view.dart: refactored with a non null resolver
* tile_layer.dart: using a setter instead
@monsieurtanuki
renamed as "resolver" and removed "immutable"
7882b07 
Impacted files:
* tile_coordinates.dart: renamed as TileCoordinatesResolver and made const
* tile_image_manager.dart: removed the immutable metatag; used a setter and refactored
* tile_image_view.dart: refactored with a non null resolver
* tile_layer.dart: using a setter instead
@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup @josxha Actually TileCoordinatesSimplifier isn't only used in TileImageManager, but also in TileImageView.
That said, removing @immutable enables me to code as I would have in a first approach, cf. latest push.

@monsieurtanuki
Copy link
Contributor Author

I have the feeling that the fling animation has stutters when using a map with longitude repetition. Most notifable when crossing the -180/180 longitude. Could anyone check if it happens to them as well?

I suspect that my latest push made things slightly faster, with const classes and fields. Not a big user of flings, but haven't noticed anything problematic.

@josxha
Copy link
Contributor

josxha commented Nov 16, 2024

Do you know why the map is offset between zoom level 1 and 2?
I've seen some thin black lines shimmering through from the custom tile builder. Do you know if it is related to this pr?

recoding.mp4

josxha
josxha approved these changes Nov 17, 2024
View reviewed changes
Copy link
Contributor

@josxha josxha left a comment

Choose a reason for hiding this comment

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

Removing the example page, solves my open points. 😄
Looks good! Thanks for the contribution.

@monsieurtanuki
Copy link
Contributor Author

@JaffaKetchup @josxha Thank you for your reviews and comments!

Still, there seems to be an issue with the "Build Android Example App" CI/CD. A bit cryptic to me: maybe the gradle version, maybe the android ndk version.

@JaffaKetchup
Copy link
Member

#1985 fixes the build for Android (would appreciate a review @josxha!), then I can fix this.

@JaffaKetchup JaffaKetchup merged commit d816b4d into fleaflet:master Nov 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arneke arneke arneke left review comments

@JaffaKetchup JaffaKetchup JaffaKetchup approved these changes

@josxha josxha josxha approved these changes

@mootw mootw Awaiting requested review from mootw

@ibrierley ibrierley Awaiting requested review from ibrierley

@TesteurManiak TesteurManiak Awaiting requested review from TesteurManiak

Assignees
No one assigned
Labels
None yet
Projects
Development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] unbounded horizontal scroll breaks most tile grids
4 participants
@monsieurtanuki @arneke @josxha @JaffaKetchup