Repository navigation
XYZ Tiles provider - #209
naulan-chrzaszcz wants to merge 1 commit into
Conversation
|
This feature is pretty raw; it was a prototype I quickly put together while I was in Albania, using the most powerful Claude Code model. |
|
I did not read the code in details, but just from a quick overview, it seems like you can quite drastically simplify it, especially by removing the re-projection. In the WMS provider, we have a CRS in the params because we need to tell to the GeoServer in which CRS we want our data. It is intended that the user inputs either the native data CRS (so the server don't reproject and our In your case, it seems like the data will always be in Additionally, keep in mind that it is completely OK for a provider to return multiple results. Even with matrices, it looks like you could return one matrix per tile instead of having to handle the merging yourself. Claude might be good at writing working code, but it's up to you to integrate it properly into the codebase (because it doesn't know about everything that is already handled by another component). If you were to write such code by hand, would you have really implemented re-projection and tile merging yourself? Probably not, because you would have known that it was tedious to do and also unnecessary ;) |
|
This feature is interesting. You should not limit it to Terrarium, many other data are provided using XYZ tiles. And as said Theo, this could be very simplified and made more Voxatile-like:
|
[Maven Build Status]📑 Commit: 📦 Download artifact: voxatile.jar |
f0860db to
95bae7f
Compare
5f62b48 to
ec221a7
Compare
| double maxY = data.offsetY() + data.sizeY() * data.cellSizeY() - 1.0; | ||
| double maxX = data.offsetX() + (data.sizeX() - 1) * data.cellSizeX(); | ||
| double maxY = data.offsetY() + (data.sizeY() - 1) * data.cellSizeY(); | ||
|
|
There was a problem hiding this comment.
Hotfix for WG84 CRS
| float x = (float) ((coordinates.x() - data.offsetX()) / data.cellSizeX()); | ||
| float y = (float) ((coordinates.y() - data.offsetY()) / data.cellSizeY()); |
There was a problem hiding this comment.
Hotfix for WG84 CRS
| <!-- TwelveMonkeys library for WebP support in ImageIO --> | ||
| <dependency> | ||
| <groupId>com.twelvemonkeys.imageio</groupId> | ||
| <artifactId>imageio-webp</artifactId> | ||
| <version>3.9.4</version> | ||
| </dependency> |
There was a problem hiding this comment.
The new endpoint of Mapterhorn use WebP.
|
Feel free to give some reviews if you'd like, @ignfab/minalac |
| /** | ||
| * Interface for decoding RGB values to float elevation. | ||
| */ | ||
| public interface ElevationDecoder { |
There was a problem hiding this comment.
Pourquoi pas plus généralement "FloatDecoder" ?
Et puis pourquoi ne pas utiliser une IntToDoubleFunction ?
There was a problem hiding this comment.
... et surtout pourquoi répondre aux questions par du code ?
There was a problem hiding this comment.
👀 Qu'est-ce que je peux dire ? Oui, je suis d'accord.
Ça fait une interface en moins... Je ne peux pas défendre ma version s'il y a évidemment mieux.
There was a problem hiding this comment.
Tu aurais pu répondre "parce que je veux du Float et pas du Double"
ec221a7 to
e2c12fe
Compare
e2c12fe to
67fdc42
Compare
| /** | ||
| * Zoom level for the tiles (required). | ||
| */ | ||
| public int zoom; |
There was a problem hiding this comment.
Do you have any usecase for that "zoom" parameter ?
In what case would we need to set a zoom value that does not fit our voxel size ?
There was a problem hiding this comment.
Zoom field is only used to defined details of the data. I explained in my JavaDoc
There was a problem hiding this comment.
That explains what "zoom" is (which is quite obvious) but not why would I choose a zoom value that does not fit voxel size.
There was a problem hiding this comment.
Je te la refais en français:
Pourquoi pourrait on choisir le niveau de zoom ? Dans quel cas cela peut être utile de choisir un autre niveau de zoom que celui qui est adapté pour le rendu ?
Je ne vois pas de cas d'usage.
67fdc42 to
ca8e6b2
Compare
|
|
||
| **Extra parameters**: | ||
| - `url` (required): URL containing the `{z}`, `{x}`, and `{y}` placeholders. Example : `https://tiles.mapterhorn.com/{z}/{x}/{y}.webp` | ||
| - `zoom` (required): Zoom level for the tiles (must be a non-negative integer) |
There was a problem hiding this comment.
Why we have to specify that ? What zoom value should I put ?
Voxatile should know better than me what zoom to use.
|
Okay, this is promising. Let's see some high-level considerations first. So, after reading your code, here is how I see it: First of all, is that correct? Let me know if I missed something important. Now, when I think about this topic, I see a different approach: So, now I have a few questions for you:
Please note that I'm asking you to write a message with your answers, not to code anything right now! |
| the formulas below are based on the Slippy map tilenames convention | ||
| https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames#Common_programming_languages | ||
| */ | ||
| private double tile2lon(int x, int z) { |
There was a problem hiding this comment.
| private double tile2lon(int x, int z) { | |
| private double xToLongitude(int x, int z) { |
| return x * 360.0 / (1 << z) - 180.0; | ||
| } | ||
|
|
||
| private int lon2tileX(double lon, int z) { |
There was a problem hiding this comment.
| private int lon2tileX(double lon, int z) { | |
| private int longitudeToX(double lon, int z) { |
| /** | ||
| * Creates a new {@code XYZTilesProvider}. | ||
| * | ||
| * @param url URL with {@code {z}}, {@code {x}}, and {@code {y}} placeholders |
There was a problem hiding this comment.
| * @param url URL with {@code {z}}, {@code {x}}, and {@code {y}} placeholders | |
| * @param url URL with {@code {x}}, {@code {y}}, and {@code {z}} placeholders |
| */ | ||
| public class XYZTilesProviderParams extends ProviderParams { | ||
| /** | ||
| * URL carrying the {@code {z}}, {@code {x}} and {@code {y}} placeholders (required). |
There was a problem hiding this comment.
| * URL carrying the {@code {z}}, {@code {x}} and {@code {y}} placeholders (required). | |
| * URL carrying the {@code {x}}, {@code {y}} and {@code {z}} placeholders (required). |
This wacky order is everywhere ...
| /** | ||
| * Constructs a new {@link XYZTilesProviderParams} with mandatory fields. | ||
| * | ||
| * @param url URL carrying the {@code {z}}, {@code {x}} and {@code {y}} placeholders |
That's important to say :D Your proposal is interesting (and promising). But before that, I think there is a serious issue with zoom that should be fixed. |
| the formulas below are based on the Slippy map tilenames convention | ||
| https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames#Common_programming_languages | ||
| */ | ||
| private double tile2lon(int x, int z) { |
There was a problem hiding this comment.
tile2lon, lon2tileX, tile2lat, lat2tileY are actually static. But that's too bad.
If you pay attention at z value passed to them, you will discover that you can remove this argument (and keep these methods non static).
|
|
||
| List<FloatGeographicDataMatrix2d> tiles = new ArrayList<>(); | ||
| for (int y = minY; y <= maxY; y++) | ||
| for (int x = minX; x <= maxX; x++) { |
There was a problem hiding this comment.
How will your code behave in Fidji Islands ?
What will occur around 180° longitude ?
Indeed, this is completely unrelated. Whatever the global design is, the same issues about the actual XYZ providing logic will need to be considered, such as the zoom parameter you mentioned, or the tiles streaming instead of buffering... but those can be handled separately, either before, in parallel, or after. Considering I (only) asked for critical thinking and analysis, I think it can be performed in parallel to addressing initial code reviews, yes! |
|
Just a side note regarding the analysis I asked you, if it's easier for you, I don't mind if you do it in French. Doing it in English would be a good exercise as well, but I prefer you focus on quality of content rather than language. |


Changes
Add a new provider which handle a new data source which provide images given the elevations with special encoding and with XYZ Tiles endpoint.
Technical changes
Add a new provider
XYZTilesProviderwith her parameters, which download images and store it into a newFloatBufferedImageMatrixmodel which can handle many format to get elevations.This model can only supported (for now) the terrarium format handle by
TerrariumElevationDecoderclass.Showcase
Reason
For Albanian and world wide DTM rendering.
TODOs
Self-checks
/docsfolder has been updatedexamples/work the same (or have been adapted if subject to changes in this PR)