-
Notifications
You must be signed in to change notification settings - Fork 6
feat: zarr #209
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
base: main
Are you sure you want to change the base?
feat: zarr #209
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you, this is awesome and I'm excited to dig in to it with you.
Next, this is just too big for a single PR. We should work to split this into more bite size PRs:
- Ideally we should be able to split any changes to deck.gl-geotiff outside of this PR
Location for zarr-multiscale-metadata
I'm not sure if you have any opinions over where zarr-multiscale-metadata should live as a package? It seems general enough to live in a repo outside of a purpose-focused deck.gl rendering solution.
I created #210 in case you want to discuss this more outside of the context of this PR. Perhaps it would make sense to have a standalone zarrita-multiscales repo? Is there any appetite for moving it into zarrita directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes related to this PR? These changes are in deck.gl-geotiff, but I wouldn't expect that any of deck.gl-zarr would depend on deck.gl-geotiff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes could make sense, but best to discuss in a standalone PR I think
| // Register EPSG:3857 (Web Mercator) - not included in proj4 by default | ||
| proj4.defs( | ||
| "EPSG:3857", | ||
| "+proj=merc +a=6378137 +b=6378137 +lat_ts=0 +lon_0=0 +x_0=0 +y_0=0 +k=1 +units=m +nadgrids=@null +wktext +no_defs +type=crs", | ||
| ); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to think through this.
- Feels like some of this reprojection should be handled in the vertex shader rather than the fragment shader
- The intent of the raster-reproject module was that we'd never have to do complex fragment shader reprojection like this.
I'd like to first support rendering without this GPU-based reprojection, and later on we can come back to this as an enhancement.
This means that we can go down two paths concurrently:
- Work on rendering COGs from EPSG:4326, to ensure that we're rendering EPSG:4326 data correctly.
- Work on rendering Zarr from Mercator, Albers, or some other projection that is known to work on COG input data already.
Otherwise, there are too many moving parts and it's too complicated to do both of these at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, all of these LOD changes may be super valuable (the LOD is one of the areas of the code base I understand less), but we need to find a way to divorce it from the generic Zarr rendering, otherwise it's too hard to understand where the changes are coming from.
| /** | ||
| * Whether row 0 of the texture is south (true) or north (false). | ||
| * Used for shader reprojection when sourceCrs is 'EPSG:4326'. | ||
| * @default false | ||
| */ | ||
| latIsAscending?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be a concern of the raster layer. The raster layer should be entirely divorced of any sort of reprojection.
Shouldn't this be determined by the mesh orientation?
| /** | ||
| * Source CRS for shader-based reprojection bypass. | ||
| * When set to 'EPSG:4326', uses fragment shader reprojection. | ||
| * When set to 'EPSG:3857', uses a simple quad (texture is already in Web Mercator). | ||
| * When null/undefined, uses full mesh refinement with reprojectionFns. | ||
| */ | ||
| sourceCrs?: SourceCrs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deck.gl has render modes for web mercator, globe, and has the ground work for generic projections. I really want to support arbitrary projections in the future.
So I want to be careful about not embedding expectations that we're either in Web Mercator rendering or in globe rendering.
| // Fast path for EPSG:4326 and EPSG:3857 source data - no mesh refinement needed | ||
| if (sourceCrs === "EPSG:4326" || sourceCrs === "EPSG:3857") { | ||
| if (!bounds) { | ||
| throw new Error( | ||
| `bounds prop is required when sourceCrs is '${sourceCrs}'`, | ||
| ); | ||
| } | ||
|
|
||
| if (sourceCrs === "EPSG:4326") { | ||
| // EPSG:4326: Simple quad + fragment shader reprojection. | ||
| // The shader uses position_commonspace to compute the fragment's | ||
| // relative position within the tile, then converts to latitude. | ||
| const mesh = generateSimpleQuadMesh(bounds, latIsAscending); | ||
|
|
||
| const effectiveLatBounds: [number, number] = latBounds ?? [ | ||
| bounds.south, | ||
| bounds.north, | ||
| ]; | ||
| const reproject4326Props = { | ||
| latBounds: effectiveLatBounds, | ||
| mercatorYBounds: [ | ||
| latToMercatorNorm(effectiveLatBounds[1]), // north | ||
| latToMercatorNorm(effectiveLatBounds[0]), // south | ||
| ] as [number, number], | ||
| latIsAscending, | ||
| }; | ||
|
|
||
| this.setState({ | ||
| reprojector: undefined, | ||
| mesh, | ||
| useReproject4326: true, | ||
| reproject4326Props, | ||
| }); | ||
| } else { | ||
| // EPSG:3857: Simple quad with no reprojection needed. | ||
| // The texture is already in Mercator space, and deck.gl displays in Mercator, | ||
| // so the non-linear projection deck.gl applies to WGS84 vertices matches | ||
| // the Mercator texture layout exactly. | ||
| const mesh = generateSimpleQuadMesh(bounds, latIsAscending); | ||
|
|
||
| this.setState({ | ||
| reprojector: undefined, | ||
| mesh, | ||
| useReproject4326: false, | ||
| reproject4326Props: undefined, | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Full mesh refinement for other CRS | ||
| if (!reprojectionFns) { | ||
| throw new Error( | ||
| "reprojectionFns prop is required when sourceCrs is not set", | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all needs to be a self-contained PR. We can discuss this more in #153
If it would help, I can make some COGs in 3857 and 4326 for test purposes. Then we could test out these projection changes while separating out any Zarr work.
| /** | ||
| * Generate a simple 4-vertex quad mesh for EPSG:3857 source data. | ||
| * | ||
| * For Web Mercator textures displayed on a Web Mercator map (deck.gl's default | ||
| * WebMercatorViewport), no reprojection is needed. GPU rasterization interpolates | ||
| * UVs in screen space, which is Mercator space after projection, so sampling is | ||
| * linear in Mercator Y - matching the Mercator texture layout. | ||
| * | ||
| * Note: This assumes WebMercatorViewport. Non-Mercator viewports (GlobeView, | ||
| * OrthographicView) would require different handling. | ||
| * | ||
| * @param bounds Geographic bounds in WGS84 | ||
| * @param latIsAscending Whether texture row 0 is south (affects UV mapping) | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could make some utilities in the raster-reproject package for generating a quad mesh. That could then be self contained and outside of the RasterLayer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to not have this vendored twice in the repo. Maybe it makes sense to have some sort of projection helpers defined somewhere that both deck.gl-geotiff and deck.gl-zarr can use. But I'd like for deck.gl-raster to not take a dependency on proj4
| // Re-export commonly used types from zarr-metadata | ||
| export type { | ||
| Bounds, | ||
| CRSInfo, | ||
| FormatDescriptor, | ||
| MultiscaleFormat, | ||
| TileConvention, | ||
| ZarrArrayMetadata, | ||
| ZarrLevelMetadata, | ||
| ZarrMultiscaleMetadata, | ||
| } from "zarr-multiscale-metadata"; | ||
| // Re-export zarr-metadata functions for convenience | ||
| export { | ||
| createFormatDescriptor, | ||
| createZarritaRoot, | ||
| loadCoordinateBounds, | ||
| parseZarrMetadata, | ||
| STANDARD_CRS, | ||
| TILED_BOUNDS, | ||
| } from "zarr-multiscale-metadata"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, this is a lot to be re-exporting from here. Ideally we can make zarr-multiscale-metadata a standalone package, and then any downstream app can depend on both deck.gl-zarr and also zarr-multiscale-metadata if they need the types.
| /** | ||
| * Result of parsing Zarr metadata for tile matrix set construction | ||
| */ | ||
| export interface ZarrTileMatrixSetResult { | ||
| /** The tile matrix set */ | ||
| tileMatrixSet: TileMatrixSet; | ||
| /** Sorted levels mapping TMS index to Zarr path */ | ||
| sortedLevels: SortedLevel[]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere (not sure if in an issue or in this PR) would be cleaner, it would be nice to have an overview of how your Zarr parsing to rendering is set up.
I heard that GeoZarr/multiscales was linked to the TileMatrixSet spec, but is no longer. So going forward multiscales will be entirely separate. In that case, I'm not sure whether it's best long-term to try to parse Zarr into TMS or to make another Tileset2D instance specifically for Zarr's tileset definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a great place to start with bite-size Zarr PRs.
-
We discuss and figure out the tileset abstraction we want to use for Zarr rendering. Is it trying to shoehorn into TileMatrixSet, or is it trying to use the bleeding edge GeoZarr/multiscales state of the art?
-
Then we add parsers and test cases specifically for that tileset metadata. Then we can discuss and merge PRs without any sort of visualization yet; just with Zarr tileset traversal.
-
At this stage, it would be great to have a Zarr "debug application", that just renders boxes for each Zarr chunk, printing the chunk id (like
0.3.2.4, or something), so we can explore and visually verify how the Zarr traversal is working, before we touch any of the Zarr data rendering itself.Another nice thing here is that this isolates the Zarr chunk traversal with the Zarr chunk reprojection. We don't need to touch any of the EPSG:3857/EPSG:4326 rendering behavior. All we care about at this stage is verifying that chunks are chosen.
-
Then we move onto Zarr reprojection (e.g. rendering a debug mesh)
-
Then we move onto Zarr data rendering
|
Thanks so much for the thorough look Kyle! Totally agree that this has gotten too big for a single PR. I got a little carried away with trying to get the demos looking good and ended up going in too many directions. I'll work on breaking things out into separate bits. |

WIP.