-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Handle rotated, skewed or flipped GeoTIFF tile grids #15402
base: main
Are you sure you want to change the base?
Conversation
📦 Preview the website for this branch here: https://deploy-preview-15402--ol-site.netlify.app/. |
fba39e7
to
fa3f0ac
Compare
Ready for review as I do not have any files small enough to use for a rotated or skewed rendering test, although the test file from #15428 would be suitable. The Projection setMatrix method should work equally well for a rotated ImageStatic, etc. but I have not documented that as part of the API. |
📦 Preview the website for this branch here: https://deploy-preview-15402--ol-site.netlify.app/. |
e8c58cd
to
9013e51
Compare
2488417
to
5b56e2a
Compare
@mike-000 Thanks for this! I'm happy to provide more test files, though the smallest I have is ~80MB. |
@loganwilliams There is a large selection of open data available via https://radiantearth.github.io/stac-browser/#/external/s3.us-west-2.amazonaws.com/umbra-open-data-catalog/stac/catalog.json typically around that size suitable for use in examples but too big to use as local test data. I have now found a reduced version of the file used in the example at https://github.com/GeoTIFF/test-data which is under 4MB so might be suitable for another rendering test. |
7030605
to
7cc890b
Compare
This is really great, thanks for your work on it @mike-000. Hope it gets merged soon. |
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.
The approach developed in this pull request is good. My suggestions are only about where parts of the implementation belong to, so more users can benefit from it.
src/ol/source/GeoTIFF.js
Outdated
/** | ||
* Replacement for GeoTIFFImage.getResolution() to handle ModelTransformation. | ||
* @param {GeoTIFFImage} image The image. | ||
* @param {GeoTIFFImage} [referenceImage] The reference image. | ||
* @return {Array<number>} The map x and y units per pixel. | ||
*/ | ||
function getResolution(image, referenceImage) { |
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.
Wouldn't it make more sense to contribute this to geotiff.js?
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 agree, if geotiffjs/geotiff.js#413 and geotiffjs/geotiff.js#414 are accepted and included in a release then these copies could be replaced by the real thing.
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.
Both are now merged and released.
src/ol/proj.js
Outdated
if (sourceMatrix || destinationMatrix) { | ||
const transform = transformFunc; | ||
sourceMatrix = invertMatrix((sourceMatrix || createMatrix()).slice()); | ||
destinationMatrix = destinationMatrix || createMatrix(); | ||
const coordTransform = function (coordinate) { | ||
return applyMatrix( | ||
destinationMatrix, | ||
transform(applyMatrix(sourceMatrix, coordinate.slice())), | ||
); | ||
}; | ||
transformFunc = createTransformFromCoordinateTransform(coordTransform); | ||
} |
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 wouldn't bake matrix transforms into projections. It does not feel right. Instead, a matrix transform should be defined with custom transform functions using addCoordinateTransforms()
, like e.g. shown in https://openlayers.org/en/latest/examples/wms-custom-proj.html.
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.
The problem with that approach is transforms would need to be maintained between every matrix variation and every projection added (past and future) via proj4. Given that this is a relatively unusual situation (although it could also be applied to KML ground overlays) I think setting an additional property on the projection and calculating the transforms dynamically as required would be easier.
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.
When you create a transform function, you can mix in the matrix transform from a separate function or variable, so you can still handle matrix variations outside the once created transform function. Basically the same thing you're currently doing in src/ol/proj.js.
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.
The existing transform functions are indexed by projection codes - the matrixes are assigned to projection objects with those codes. Doing it this way avoids adding complexity to the indexing of the primary transforms.
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 you point me to the code section where that happens?
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.
The new code calls projection.setMatrix(matrix);
or this.projection.setMatrix(matrix);
when determining the projection https://github.com/mike-000/openlayers/blob/7cc890b34747feb99173b974d2e6e143c88e0504/src/ol/source/GeoTIFF.js#L297
and if a matrix is detected when an override projection has been specified
https://github.com/mike-000/openlayers/blob/7cc890b34747feb99173b974d2e6e143c88e0504/src/ol/source/GeoTIFF.js#615
It is handled in existing reprojection code via getTransform
and transform
calls which need no changes as they are passed the projection objects (not codes) and therefore pass them on unchanged to getTransformFromProjections
.
7af1d67
to
e42655e
Compare
eb6ce2c
to
8c33271
Compare
@ahocevar This is ready for further review. Workarounds have been removed in favour of the latest geotiff.js methods. The handling of matrixes in projections now uses entire transform functions (in series if necessary) instead of very inefficiently extracting a single coordinate transform from a projection transform function to use in a new transform function. This has made the rendering noticeably faster. In many cases (as in the two examples and current tests) the matrix transform is independent of projection transforms and could potentially be cached and/or used directly, e.g.
although I suspect there would be little further performance gain from this. Matrix and projection transforms used in series will be needed for generic viewers such as attempted in https://radiantearth.github.io/stac-browser/#/external/s3.us-west-2.amazonaws.com/umbra-open-data-catalog/stac/2023/2023-02/2023-02-12/6d584f33-0489-47dd-9412-14d2c83532fc/6d584f33-0489-47dd-9412-14d2c83532fc.json where the rotated UTM is further transformed to 3857 as in https://jsfiddle.net/w4qxs50k/ - a test is still needed for that (potentially using an ImageStatic as in #7255 (comment)). |
}, | ||
); | ||
} | ||
return ( |
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.
A single transform could be used directly here, but any performance gain from the extra code would be minimal
if ((!sourceTransform || !destinationTransform) && !transformFunc) {
return sourceTransform || destinationTransform;
}
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.
Thanks for your continued effort here, @mike-000. The one thing that still does not look right is that you're using Projection
as container for the transform matrix, and the projection transform function to apply a transform matrix. Like I said already, that does not feel like the right place.
Tthinking about it more, my previous suggestion to use transform functions directly does not feel right.
The GeoTIFF
source looks like the right place to maintain a mapping image -> matrix, and to make use of it in an overridden getTile()
method.
Separating matrix and projection transforms has the potential for double reprojection. I would regard the image as being in a rotated version of a standard projection - in the example case a UTM projection - defined by a standard projection code and a rotation matrix, so to render it in for example EPSG:3857 only one reprojection should be needed (no reprojection of reproj tiles) the transform function being a combination of the UTM to 3857 transform and the matrix transform which can be easily done by Similarly the technique could be used for KML ground overlays, by regarding them as being in rotated/scaled version of EPSG:4326, they could be easily rendered in any view projection which has a transform to EPSG:4326. But I do think setting a matrix on a projection object should not be exposed in the API, it should be done only inside the GeoTIFF source code (and potentially inside a ground overlay parser) to avoid any future breaking changes. |
I agree that a single transform of raster data is preferable. But still, this can be done in |
A solution which is not specific to DataTile would be preferable, so it could be easily reused elsewhere, e.g. as part of a fix for #4949/#7255 where rotation transforms can be combined projection transforms without the need to maintain large numbers of individual transforms for multiple projection code/rotation permutations. Also reprojection inevitably reduces the output quality, sometimes as in #15579 that can be unacceptable, so it would nice to allow the view to be in the tiles native projection and reproject any less important background or vector data. Currently |
Handle all transforms including flipped Also set matrix when projection is specified in options Avoid unnecessary nodata
…yers into geotiff-rotation
@mike-000 To make it easier for me to review, please answer one question: in Projection, is it now either matrix or transform function, or both? If both, then how does that work exactly? |
The projection code together with an optional transform matrix defines a projection. |
@mike-000 Thanks for the explanation. I wish it was the other way, i.e. either numeric transform or matrix transform. Now that it's not like that, what exactly does
mean? I stell feel that in this case, registering transforms with transform functions (without |
The process necessary to display a rotated or flipped UTM projection tile grid as in the example or in #15923 in a generic viewer such as http:https://app.geotiff.io/ which uses a global projection (or in some cases the opposite as in https://jsfiddle.net/jo9wav6y/ to reproject a background such as OSM to the camera projection). When the source is loaded we know its projection code and ModelTransformation, but we do not know the view projection and may need look up the source projection proj4 definition to obtain that part of the transform. The adjustments for ModelTransformation do not need a lookup and can easily be added on demand. |
Well, like you're saying - the whole transform needed here is a "process", not a matter of projection. If you manage to change your pull request to do the transform with a chain of transform functions (i.e. a process) instead of trying to squeeze this into |
So how do you propose to do that? Since all methods which take a source projection and destination projection eventually call |
Would you prefer to create unique projections with codes such as
(where we would still need to wait until transforms for |
I can answer that once I fully understand what you're aiming for, which is not the case. So one more question: why are you saying the transform matrix is applied after reprojecting? I would think that if a GeoTIFF is rotated, skewed or flipped, that applying a transform matrix would be the first thing to do. Then we're done, unless the result is still in a different projection than the |
Yes, that is the problem situation. Consider an OpenLayers version of https://app.geotiff.io/ where the code might include
To avoid reprojecting a reprojection, which would affect output quality, the tiles should be reprojected directly from the rotated UTM projection to the view projection - the overall transform for doing that is obtained by combining the matrix and proj4 transforms (only the proj4 transform is cached, transform functions for a matrix are easy to create as required). That is more complex than in the example where only the matrix transform is used to reproject the tiles (to an unrotated reprojection tile grid) and the proj4 transform is only needed to reproject the background OSM to UTM. |
Ok, so the matrix transform comes first, then the projection transform. So once you have a transform function that combines both, you don't have to create a transformed canvas from a transformed canvas. Try to think less in what we currently have (i.e. Projection), think more in what would make most sense. For example, a renderer could get data from a source, obtain a transform matrix from the source, get the projection from the view, and then do what we currently call "reprojection" using a function that combines the matrix transform and the reprojection transform functions. |
I would consider the projection with matrix to be a projection in its own right as a tile grid must be aligned with the source projection axes, so it does make sense to me. And as noted above
so any other method is likely to be more complex to implement (especially for something which will be relatively uncommon) and if done at renderer level would not be reusable for other layer types (e.g. a KML GroundOverlay could be easily implemented as an ImageStatic in a scaled and rotated EPSG:4326 based projection and reprojected into any view projection with minimal additional effect). Also it would be desirable to be able use the rotated source projection directly at times as in https://jsfiddle.net/jo9wav6y/ as any reprojection will affect output quality and in a rotated version of a COG such as in #15579 changing the pixel alignment would make the output even more meaningless than the unwanted resizing of pixels. |
Again, I'm not talking about multiple transforms on a canvas. Just chaining of transform functions. And my suggestion is to separate affine transforms (source matrix transform) from projections (source to view projection). An API for the user could look like this: new Source({
projection: sourceProjection,
transform: transformMatrix, // transform data into source projection
}); For GeoTIFF, both can of course be determined from the GeoTIFF metadata. |
Why make it unnecessarily complicated for the rendererers? |
We should aim for making it easy for the users. I don't see where this would make it more complicated in the renderer code. The problem with your approach is that projections work based on a projection code lookup. And with an arbitrary matrix in addition to a projection that can uniquely be identified by a code, we get into a situation where we need to set up a code (or a projection object) for each transform+projection combination. |
Yes, we need new projection objects for custom projections - before each call to |
I think I have made valid suggestions and also shown a way how these could be implemented. Please let's bury your selected approach of trying to squeeze all that into Projection objects. |
Your proposal works well when the source is reprojected to another projection, e.g. UTM source with a rotation matrix reprojected to 3857. The matrix needs to be passed to |
Maybe some things can be simplified along the way:
It sounds like that would resolve most or your issues. |
Yes, I have it mostly working now, by taking the source transform matrix into account in |
Interesting. Would you mind sharing your code? I'm unable to come up with a reason why the transform would matter for the view projection. |
Fixes #15294
Fixes #15370
Fixes #15428
Uses rotated/skewed/flipped variants of projections to handle GeoTIFF tile grids with rotation, skew or flip specified by ModelTransformation.
See https://deploy-preview-15402--ol-site.netlify.app/en/latest/examples/cog-modeltransformation.html
Uses a local alternative to geotiffjs/geotiff.js#403 which could be removed if the replacement geotiffjs/geotiff.js#413 (with corrected Y axis scale) is merged. The code could be further simplified if geotiffjs/geotiff.js#414 is merged.