-
Notifications
You must be signed in to change notification settings - Fork 55
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
Autodetect the max zoom level based on the highest resolution coverage #79
Conversation
…ghest resolution coverage in the request. - Re: issue #75
@pdavidc You're review is welcome, but not required to move forward. |
double e = Math.abs(g.getScaleDenominator() - reqScaleDenominator); | ||
if (e > error) { | ||
break; | ||
} else { |
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.
Is this conditional necessary? I think readability could be improved:
if (e > error) {
break;
}
error = e;
i++;
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 concur. Thanks.
ProblemWhen a GeoPackage-based layer is exported as a GeoPackage, the max zoom level is determined to be the max zoom level in the gridset (level 21), not the max level in the tileset. This is problematic: it creates a request for a GeoPackage at the highest possible resolution, many levels greater than required, causing a seemingly never-end job that consumes large amounts of memory and disk resources. The coverage object that provides the gridset is unaware of the actual number of levels in the tileset. Options/Ideas
|
…t) of the tiles in the GeoPackage. - Was returning the grid range of the entire grid level. - Added supporting code to compute the grid range. - Refactoring is needed. See TODO statements.
…ided by the GridCoverage2DReader. - Provides access to getOriginalGridRange for obtaining the width/height of a GeoPackage.
@pdavidc I would appreciate a sanity check on the changes while I move into functional testing. |
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 have a few questions about the handling of GeoPackage tile matrices that I'm hoping have easy answers, or highlight a potential problem.
@@ -90,21 +90,44 @@ | |||
protected File sourceFile; | |||
|
|||
protected Map<String, TileEntry> tiles = new HashMap<String, TileEntry>(); | |||
|
|||
// TODO: store these gridRanges in the TileEntry class | |||
protected Map<String, GridEnvelope2D> gridRanges = new HashMap<String, GridEnvelope2D>(); | |||
|
|||
public GeoPackageReader(Object source, Hints hints) throws IOException { | |||
coverageFactory = CoverageFactoryFinder.getGridCoverageFactory(this.hints); |
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.
It appears that the hints argument is unused. Could it be that the object property hints is unnecessary, or that it hints needs to be assigned before the call to getGridCoverageFactory?
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.
Good eyes! It appears the GeoPackageReader is intentionally using the "defaultHints" provided in the base class. The rationale for this is unknown.
// TODO: compute the gridRange in the GeoPackage class | ||
List<TileMatrix> matricies = tileset.getTileMatricies(); | ||
TileMatrix matrix = matricies.get(matricies.size() - 1); | ||
int maxZoomLevel = matrix.getZoomLevel(); |
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 are two aspects of the GeoPackage spec that could be problematic here:
-
GeoPackage does not stipulate that tile matrices are ordered, which could invalidate the assumption that the last TileMatrix represents the maximum zoom level. Does the method TileEntry.getTileMatrices provide this guarantee?
-
GeoPackage allows the definition of tile matrices that have no corresponding tile data in the file. For example, a GeoPackage could define 17 logical levels of tile matrices, yet provide tile data for only levels 5-10. We have test files that do this. In WorldWind Android, I queried the GeoPackage to identify the TileMatrix with the largest zoom level that also had at least one corresponding tile in the tile table.
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.
Re: 1. The GeoPackage class retrieves and assembles its tile matrices in ascending zoom level order.
Re: 2. The query behind the getTileBound method is on "coverage tiles" table, not the tile_matrix table.
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.
To clarify, "coverage tiles table" refers to GeoPackage tile pyramid user data tables?
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.
Yes, that is correct.
GridEnvelope2D gridRange = new GridEnvelope2D(new Rectangle( | ||
numCols * matrix.getTileWidth(), | ||
numRows * matrix.getTileHeight())); | ||
|
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 conversion to GridEnvelope2D appears to lose information. This retains the tile count in each dimension, but drops which row and column the envelope begins at. Have I missed something here?
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 conversion is storing the height and width in pixels of the highest resolution level.
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.
It seems odd that the Rectangle's upper left corner would be set to 0, 0. Are you certain that getOriginalGridRange, and any other use of this GridEnvelope2D, will only inspect its dimensions?
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'm been looking into it since your original comment. So far nearly all instances have been on the span, but its a large domain to inspect. I cannot identify an alternative approach to manifesting this data, nor can I propose candidate values for a top/left in this context other than 0,0.
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.
It appears that the domain is logical tile matrix pixels. Given that, would minCol * matrix.getTileWidth()
and minRow * matrix.getTileHeight()
represent the upper left x and y, respectively?
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.
On second thought, the low/high could be the pixels in the grid. 2^21 * 256 fits in an integer.
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.
@emxsys many of the details of the context of this pull request are out of the scope of my Geoserver knowledge :) I focused on the zoom level determination and found no issues in your approach. My spreadsheet backed testing indicated the closest level + 1 would be returned for a given scale denominator.
Overriding findMaxZoomAuto to return the zoom level closest to the highest resolution coverage in the request.