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

Optimize build, spelling, linting #56

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Optimize build, spelling, linting #56

merged 1 commit into from
Jun 15, 2023

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jun 13, 2023

  • Remove duplicate "is_empty" test - if the iteration is empty, it will produce an empty vec very quickly.
  • remove unneeded double referencing for the format (eprintln)

See also shssoichiro/oxipng#519

Copy link
Owner

@flother flother left a comment

Choose a reason for hiding this comment

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

Thanks for this audit of the Spreet codebase, it's always gratifying to see people offer improvements to my code.

I'm a big fan of atomic commits, so if you have the time I'd love to see this split into three separate PRs:

  • Oxipng change
  • Removing the duplicate is_empty test
  • The other changes to src/bin/spreet/main.rs

(It could equally be a fresh PR with atomic commits that I can merge rather than squash-and-merge.) I hope that doesn't seem too onerous. Then I can merge them and release v0.8.0. What do you think?

For the other changes, see comments below.

Cargo.toml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Cargo.toml Outdated
crunch = "0.5.3"
exitcode = "1.1"
multimap = "0.9"
oxipng = "8"
oxipng = { version = "8.0", features = ["parallel", "zopfli", "filetime"], default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

Love it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this to another PR

* Remove duplicate "is_empty" test - if the iteration is empty, it will produce an empty vec very quickly.
* remove unneeded double referencing for the format (eprintln)
@nyurik
Copy link
Contributor Author

nyurik commented Jun 15, 2023

thx, moved to #61

@flother flother merged commit 5efd73b into flother:master Jun 15, 2023
nyurik added a commit to maplibre/martin that referenced this pull request Jun 16, 2023
Dynamically create image sprites for MapLibre rendering, given a
directory with images.

### TODO
* [x] Work with @flother to merge these PRs
  * [x] flother/spreet#59  (must have)
  * [x] flother/spreet#57
  * [x] flother/spreet#56
* [ ] flother/spreet#62 (not required but nice
to have, can upgrade later without any code changes)
* [x] Add docs to the book
* [x] Add CLI param, e.g. `--sprite <dir_path>`
* [x] Don't output `.sprites` in auto-genned config when not in use

### API
Per [MapLibre sprites
API](https://maplibre.org/maplibre-style-spec/sprite/), we need to
support the following:
* `/sprite/<sprite_id>.json` metadata about the sprite file - all coming
from a single directory
* `/sprite/<sprite_id>.png` all images combined into a single PNG
* `/sprite/<sprite_id>@2x.json` same but for high DPI devices
* `/sprite/<sprite_id>@2x.png`

Multiple sprite_id values can be combined into one sprite with the same
pattern as for tile joining:
`/sprite/<sprite_id1>,<sprite_id2>,...,<sprite_idN>[.json|.png|@2x.json|@2x.png]`.
No ID renaming is done, so identical names will override one another.

### Configuration
[Config file](https://maplibre.org/martin/config-file.html) and possibly
CLI should have a simple option to serve sprites. The configuration may
look similar to how mbtiles and pmtiles are configured:

```yaml
# Publish sprite images
sprites:
  paths:
    # scan this whole dir, matching all image files, and publishing it as "my_images" sprite source
    - /path/to/my_images
  sources:
    # named source matching source name to a directory
    my_sprites: /path/to/some_dir
```

Implement #705
@nyurik nyurik deleted the fixes branch October 12, 2023 06:28
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.

2 participants