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

When generating the seed list, we need the grid for the bounds #1024

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

gdey
Copy link
Member

@gdey gdey commented Dec 24, 2024

Our old system normalized the bounds to grid, this meant that people could provide a bounds without the associated srid as long as it was either 4326 or 3853. With the change over, we were defaulting to 3853. Added option to allow for the grid to be 4326.

@gdey gdey requested a review from iwpnd December 24, 2024 04:11
@gdey gdey requested a review from ARolek as a code owner December 24, 2024 04:11
@gdey gdey mentioned this pull request Dec 24, 2024
@coveralls
Copy link

coveralls commented Dec 24, 2024

Pull Request Test Coverage Report for Build ff3bd7f59

Details

  • 6 of 17 (35.29%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 42.851%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cmd/tegola/cmd/cache/seed_purge.go 6 17 35.29%
Totals Coverage Status
Change from base Build 60841eccb: -0.01%
Covered Lines: 7034
Relevant Lines: 16415

💛 - Coveralls

@gdey gdey force-pushed the add_ability_to_specify_grid_for_cache_cmd branch from 6c3eef0 to 6d61072 Compare December 24, 2024 04:22
Copy link
Member

@iwpnd iwpnd left a comment

Choose a reason for hiding this comment

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

yessss, that looks better! 🚀

@gdey gdey force-pushed the add_ability_to_specify_grid_for_cache_cmd branch from 6d61072 to c765511 Compare December 24, 2024 19:33
Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

@gdey thanks for digging into this! You found the problem, but I'm wondering if the default behavior is the opposite of what we want. Up until this bug surfaced the bounds was always expected to be in 4326. With this change we now require all the users to add --wgs84=true when seeding. It seems we should default to wgs84. Thoughts?

@gdey
Copy link
Member Author

gdey commented Dec 26, 2024

@gdey thanks for digging into this! You found the problem, but I'm wondering if the default behavior is the opposite of what we want. Up until this bug surfaced the bounds was always expected to be in 4326. With this change we now require all the users to add --wgs84=true when seeding. It seems we should default to wgs84. Thoughts?

If 4326 was the default then i agree we should swap it.
I did not realize that when i first fix the bug, because we our math was slightly off and we had a bunch of small adjustments. This came from me not fully understanding things at the time i built it.

@gdey
Copy link
Member Author

gdey commented Dec 31, 2024

After discussion we decided it would be better to provide a flag to state the SRID of the bounds.

cache seed --help
command to seed or purge tiles from the cache

Usage:
  tegola cache seed [flags]
  tegola cache seed [command]

Aliases:
  seed, purge

Examples:
  tegola cache seed --bounds lng,lat,lng,lat

Available Commands:
  tile-list   operate on a list of tile names separated by new lines
  tile-name   operate on a single tile formatted according to --format

Flags:
      --bounds string       lng/lat bounds to seed the cache with in the format: minx, miny, maxx, maxy (default "-180,-85.0511,180,85.0511")
      --bounds-srid int     the srid of the grid system for bounds. (default 4326)
      --concurrency int     the amount of concurrency to use. defaults to the number of CPUs on the machine (default 12)
  -h, --help                help for seed
      --log-threshold int   during seeding, only log tiles that take this number of milliseconds or longer to render (default all tiles)
      --map string          map name as defined in the config
      --max-zoom uint       max zoom to seed cache to (default 22)
      --min-zoom uint       min zoom to seed cache from
      --overwrite           overwrite the cache if a tile already exists (default false)
Global Flags:
      --config string      path or http url to a config file, or "-" for stdin (default "config.toml")
      --log-level string   set log level to: TRACE, DEBUG, INFO, WARN or ERROR (default "INFO")
      --logger string      set logger to: standard, zap - default: standard (default "standard")
Use "tegola cache seed [command] --help" for more information about a command.

@gdey gdey force-pushed the add_ability_to_specify_grid_for_cache_cmd branch from ff3bd7f to b9dab62 Compare December 31, 2024 22:06
gdey added 2 commits December 31, 2024 14:09
* updated proj to v0.3.0
* update golang.org/x/net/html
Added the ability to specify the srid to use. It defaults to 4326.

Our old system normalized the bounds to grid, this meant that people could provide a bounds without the associated srid as
long as it was either 4326 or 3853. With the change over, we
were defaulting to 3853. This was the wrong default. Chanced the default 4326, and added ability to specify the srid.

We will check to see if the SRID is supported by proj. For now the only srid's that are supported are 4326 and 3853.
@gdey gdey force-pushed the add_ability_to_specify_grid_for_cache_cmd branch from b9dab62 to 141b759 Compare December 31, 2024 22:12
Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

LGTM!

@gdey gdey merged commit 3537c97 into master Dec 31, 2024
16 checks passed
@gdey gdey deleted the add_ability_to_specify_grid_for_cache_cmd branch December 31, 2024 22:15
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.

4 participants