-
Notifications
You must be signed in to change notification settings - Fork 22
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
[WIP] Make the mosaic map endpoint more interactive #89
Conversation
The tests are failing because these changes require titiler>=0.11.7 but titiler-pgstac requires titiler<11. Updates to titile-pgstac to allow titiler>11 are in progress here: stac-utils/titiler-pgstac#93. |
@hrodmn I'm sorry I haven't looked at this PR earlier. I fixed some dependency issue and added by default the |
@vincentsarago no worries! And thanks for letting me know about the recent updates. I got a rudimentary version of the asset picking UI working a few weeks ago but had to put it down in favor of other work. I'll take a look at your recent updates later today. |
34b13cc
to
079906b
Compare
############################################################################### | ||
# MOSAIC Endpoints | ||
# https://github.com/developmentseed/titiler/blob/main/src/titiler/extensions/titiler/extensions/viewer.py#LL21C1-L42C1 | ||
@dataclass | ||
class mosaicViewerExtension(FactoryExtension): |
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.
can we put this in an extension.py
submodule? (helps to keep app clean)
extensions=[ | ||
mosaicViewerExtension(), | ||
], | ||
layer_dependency=AssetsBidxExprParamsOptional, |
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.
why do we need to change this?
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 way you can navigate to the /map
endpoint without specifying any parameters. Before I made this change I had to specify some parameters in the url in order to view the map (e.g. .../map?assets=layer_name
). If I did not specify the layers I got this error message: {"detail":"assets must be defined either via expression or assets options."}
.
Now, when you navigate to the /map
endpoint you land in the mosaic browser web map and use the interactive menu to load specific layers rather than encoding them in the URL.
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.
that's pretty dangerous
because the layer_dependency
is also used in other endpoints. I think it will be better to not change it and use a custom parameter in the extension
(I can do it if needed)
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.
Ah I see what you mean. I'll try to find an alternative way around it!
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.
@vincentsarago can we drop the layer_dependency
arg from the register()
method for the new extension? Or should we have it set to a different value there rather than inheriting from the parent factory
?
https://github.com/ncx-co/eoAPI/blob/db40b59c2ef0b0c26dca5ebb37c9d8a51576a115/runtime/eoapi/raster/eoapi/raster/extension.py#L58
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 was going to push some change to this PR but I have couple question:
-
why do we need to pass the tilejson url into the template? https://github.com/ncx-co/eoAPI/blob/map-endpoint-interactive/runtime/eoapi/raster/eoapi/raster/extension.py#L116. The tilejson URL will be created in webpage (https://github.com/ncx-co/eoAPI/blob/db40b59c2ef0b0c26dca5ebb37c9d8a51576a115/runtime/eoapi/raster/eoapi/raster/templates/mosaic-browser.html#L292-L303)
-
it seems to me that we could keep the original
/map
endpoint and just add a/viewer
endpoint with your new UI? -
we could then get rid of all of this https://github.com/ncx-co/eoAPI/blob/map-endpoint-interactive/runtime/eoapi/raster/eoapi/raster/extension.py#L46-L84
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.
We may not need to pass the tilejson url, that could be left over from when I was first getting oriented.
I think a separate endpoint is a great idea! I can start making the switch to a /viewer
(or /browser
?) endpoint.
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.
let's use /viewer
(it's what we use in other endpoints) 👍
435477a
to
db40b59
Compare
I am sorry about the force pushes here but I wanted to rebase this PR on main so I can make sure it is compatible with the mosaic builder! |
I am closing this out since I have not had a chance to work on it in many months - might revisit later! |
Goals:
titiler
...{searchid}/map?assets=cog&rescale=0,1000
, those parameters get pre-loadedResolves #79