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

Read crs information from metadata #114

Merged
merged 8 commits into from
Mar 26, 2024
Merged

Read crs information from metadata #114

merged 8 commits into from
Mar 26, 2024

Conversation

katamartin
Copy link
Member

@katamartin katamartin commented Mar 26, 2024

This PR updates the library to read the crs attribute in multiscales metadata [0], when present, to determine the projection the data are stored in. The existing projection prop will still take precedence and can be used as an override or to specify a value when crs is missing.

[0] https://ndpyramid.readthedocs.io/en/latest/schema.html

cc @andersy005 @maxrjones

@katamartin katamartin requested a review from Shane98c March 26, 2024 14:49
Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
maps-demo ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 4:05pm
maps-docs ⬜️ Ignored (Inspect) Visit Preview Mar 26, 2024 4:05pm

Copy link
Member

@Shane98c Shane98c left a comment

Choose a reason for hiding this comment

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

so cool that we support multiple projections here! I hadn't looked at that code until now. I just had a couple questions around handling unsupported projection/crs inputs.

Comment on lines +127 to +134
// Respect `projection` prop when provided, otherwise rely on `crs` value from metadata
this.projectionIndex = projection
? ['mercator', 'equirectangular'].indexOf(projection)
: ['EPSG:3857', 'EPSG:4326'].indexOf(crs)
this.projection = ['mercator', 'equirectangular'][
this.projectionIndex
]

Copy link
Member

Choose a reason for hiding this comment

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

looks like we default to mercator in the shader in the case of a -1 index, but is there any other reason to handle an invalid index early here? Seems like updateCamera and maybe the region picker stuff might be sad if this.projection ends up undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! Decided to add a check here to handle either an unexpected crs and projection value. Want to take another look.

Comment on lines +361 to +367
if (!crs) {
console.warn(
'Missing `crs` value in `multiscales` metadata. Please check your pyramid generation code. Falling back to `crs=EPSG:3857` (Web Mercator)'
)
crs = 'EPSG:3857'
}
return { levels, maxZoom, tileSize, crs }
Copy link
Member

Choose a reason for hiding this comment

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

might be worth adding a check to see if the provided crs is one of our two valid options and to report out those options if not

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a consolidated check in tiles.js

@katamartin katamartin requested a review from Shane98c March 26, 2024 16:05
@katamartin katamartin merged commit e94393c into main Mar 26, 2024
10 checks passed
@katamartin katamartin deleted the katamartin/crs-reading branch March 26, 2024 17:16
@katamartin katamartin mentioned this pull request Apr 2, 2024
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