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

Add pmtiles source support #39

Merged
merged 23 commits into from
Feb 19, 2024
Merged

Conversation

acalcutt
Copy link
Contributor

@acalcutt acalcutt commented Feb 17, 2024

This PR adds support for pmtiles sources. It supports both local pmtiles files and http(s) pmtiles files

It is currently a work in progress and could likely use some more cleanup

I am testing with the attached style
protomaps.json

I am using the command
node . --style self --stylelocation styles/protomaps.json --stylesources tests/fixtures/alert/sources --bounds "-79,37,-77,38" -Z 9

One thing to note is I had trouble with spites and glyphs in a local folder. It was looking for them at the 'styles/protomaps.json' path, instead of just styles and i couldn't figure out how to change that. I twaeked the code a bit to accept https urls for glyphs and sprites to get around it for testing, but i do wonder if i was doing something wrong.

@rudokemper
Copy link
Member

Thanks for this work @acalcutt! The issue with sprites and glyphs not working locally is likely a byproduct of these being largely neglected since development on this project (as a fork of https://github.com/consbio/mbgl-renderer) began; we've been focusing more on online sources and local tiles that don't use these. I likely made some changes to how paths work that broke things for local sprite and glyphs, and haven't been testing for that. I'm happy to take a look at resolving that. Would you mind sharing your local assets for me to work with?

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 17, 2024

For the local spites error I was having, I was using something like this for assets
https://wifidb.net/demo/mapgl-tile-renderer/protomaps_local_test_style.zip

With --stylelocation styles/protomaps.json it was giving errors like
Error rendering tile 2/0/2: Error: ENOENT: no such file or directory, open 'C:\Users\Andrew\Documents\GitHub\mapgl-tile-renderer\styles\protomaps.json\sprites\sprite.json'
where it was including protomaps.json as part of the path

@rudokemper
Copy link
Member

With #43 the font/sprite path problem should now be solved. However, I was reminded that I was having an issue with sprites; for my styles with sprites, MapLibre just hung indefinitely when trying to render with no error message or anything else helpful to debug. I'd be curious to see if you also have this issue.

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 18, 2024

I do seem to have the same issue you mentioned when trying to use the local sprites. it just sits and doesn't do anything.

adding in some echos into the code to see what it is doing, it seems like the paths are all correct now.

One thing I notice is the response for the sprite images seems to be delayed...you can see in this screenshot with https sprite source the callback buffers happen much closer to where they were requested
image

compared to trying to use a local sprite, it seems like it replies late after other things have been requested
image

it does look like the contents of the buffer returned are the same in both cases, which is why i'm wonding if it is returning to late or something like that. kind of similar to an issue github.com/maptiler/tileserver-gl/pull/1076 where the font was being returned to late

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 18, 2024

@rudokemper I think I might have fixed the local sprite issue. I noticed compared to other callbacks that the sprite one didn't have any brackets around the data being returned. It seems like putting those in fixed the issue for me 807229e

@acalcutt
Copy link
Contributor Author

I made a new test style for this, which uses vector tiles/raster tiles/and raster-dem for hillshade
pmtiles_test_style.zip

There are two styles included
1.) http_pmtiles_local_spites_fonts.json - this uses http demo pmtiles I have on my site for other examples. this should work with just the included sprites and fonts

2.) local_pmtiles_local_spites_fonts.json - this is formatted for local pmtiles, where the files would be in your stylesources directory, like existing mbtiles. for this you would need to download these files into your sources directory
https://wifidb.net/demo/pmtiles/sources/planetiler_2023-09-02.pmtiles
https://wifidb.net/demo/pmtiles/sources/gebco_color_releif.pmtiles
https://wifidb.net/demo/pmtiles/sources/osm_basemap.pmtiles
https://wifidb.net/demo/pmtiles/sources/gebco_terrarium0-8.pmtiles

@acalcutt acalcutt marked this pull request as ready for review February 18, 2024 16:37
@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 18, 2024

I made this file with the style from the post above
https://wifidb.net/demo/mapgl-tile-renderer/wdb_15.mbtiles
with the command
node . --style self --stylelocation styles2\local_pmtiles_local_spites_fonts.json --stylesources tests\fixtures\alert\sources --bounds "-79,37,-78,38" -Z 15 -f wdb_15

image
image

Copy link
Member

@rudokemper rudokemper left a comment

Choose a reason for hiding this comment

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

This is great. Thanks for the contribution!

I'd love to add a test. If you're up for adding one to the Jest suite, that would be welcome, but I'm happy to do it as well. (I'm also going to add sprites to the style with fonts, since this PR solved that problem too.)

Similarly and related to the resolution of the sprites issue, we can now remove these comments... which could be part of this PR, or I'm good to take it out in a separate batch of cleanup work too.

@acalcutt
Copy link
Contributor Author

acalcutt commented Feb 18, 2024

A added in some source files, which are basically just your original source files converted to pmtiles with their cli tool. I also added a style and test that should use them.

However I tried to run the jest test and it doesn't seem like it likes the pmtiles package... I'm not quite sure how to fix that

    Details:

    C:\Users\Andrew\Documents\GitHub\mapgl-tile-renderer\node_modules\pmtiles\dist\index.js:1601
    export {
    ^^^^^^

    SyntaxError: Unexpected token 'export'

      3 | import zlib from "zlib";
      4 | import MBTiles from "@mapbox/mbtiles";
    > 5 | import { PMTiles, FetchSource } from "pmtiles";
        | ^
      6 | import https from "https";
      7 |
      8 | // Validations for source URLs

      at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1505:14)
      at Object.require (src/request_resources.js:5:1)
      at Object.require (src/render_map.js:4:1)
      at Object.require (src/generate_resources.js:10:1)
      at Object.require (src/initiate.js:5:1)
      at Object.require (tests/test.js:7:1)

@rudokemper
Copy link
Member

Ah, looks like pmtiles uses ES module syntax, which jest doesn't like by default. We need to transpile it for jest. We can add this to package.json for the test to work:

  "jest": {
    "transformIgnorePatterns": [
      "node_modules/(?!(pmtiles)/)"
    ]
  }

@rudokemper
Copy link
Member

rudokemper commented Feb 18, 2024

Thanks! Let me know when you're done working on the PR, and I'll merge. (Oops, clicked the wrong button, didn't mean to close the PR.)

@rudokemper rudokemper closed this Feb 18, 2024
@rudokemper rudokemper reopened this Feb 18, 2024
@acalcutt
Copy link
Contributor Author

I think this should be ready, whenever you want to merge it

@rudokemper rudokemper merged commit d6b9884 into ConservationMetrics:main Feb 19, 2024
0 of 2 checks passed
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