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

faster rendering by looking at min and max scales before read #299

Merged
merged 3 commits into from Dec 20, 2018

Conversation

sloob
Copy link
Contributor

@sloob sloob commented Sep 28, 2018

To increase the performance of the GridCoverageReaderRenderer, raster rendering will only be performed if the map's current scale is within the min / max scale of the layer.

Signed-off-by: sloob sebastian.loob@ibykus.de

Signed-off-by: sloob <sebastian.loob@ibykus.de>
@fgdrf fgdrf added this to the 2.1.0 milestone Sep 28, 2018
@fgdrf
Copy link
Contributor

fgdrf commented Sep 28, 2018

started ci-build : https://ci.eclipse.org/udig/job/uDig-PR/30/

// faster rendering if out of scale
GridCoverageRenderState state = getRenderState(getContext());
double scale = state.context.getViewportModel().getScaleDenominator();
if (scale < state.minScale || scale > state.maxScale)
Copy link
Contributor

Choose a reason for hiding this comment

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

Great, change looks good! Any chance to add a test case?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMH minScale / maxScale of state object might be null, if one or the other is not set.

Can you explaint how this change is related to line 232/ 227 where Rule of Style is compared to teh currentScale? I guess it is possible to define different rules for the same resource and one rule might fit where another doesn't.

So maybe it makes sense to move the style rule scale comparence before creating a coverage ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

minScale / maxScale should never be null. The GeoTools implementation of Rule initializes these double values with 0.0 and Double.PositiveInfinity (see https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/styling/RuleImpl.java).

The problem is, that the SLD do not have to include a RasterSymbolizer. If there is no RasterSymbolizer, a default Symbolizer is used (line 257) and the layer is rendered.
So using the GridCoverageRenderState with the feature type style seems to be the right way.

Signed-off-by: sloob <sebastian.loob@ibykus.de>
@fgdrf
Copy link
Contributor

fgdrf commented Oct 22, 2018

Started a CI-Build for this pull request again : https://ci.eclipse.org/udig/job/uDig-PR/32/

Copy link
Contributor

@fgdrf fgdrf left a comment

Choose a reason for hiding this comment

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

Great to have unit tests! I'm wondering about different formatings of java class files - they looking different and formatter rules might not applied everywhere. I guess the comments are easy to address and I'd to merge it afterwards till the end of 2018 ;)

plugins/pom.xml Outdated
@@ -33,6 +33,7 @@
<module>org.locationtech.udig.libs.tests</module>
<module>org.locationtech.udig.location.test</module>
<module>org.locationtech.udig.render.feature.basic.test</module>
<module>org.locationtech.udig.render.gridcoverage.basic.test</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like tabs are at the beginning of the lin rather that spaces, can we fix it?

paramMap.put("name", "BasicGridCoverage2DFormat"); //$NON-NLS-1$//$NON-NLS-2$
paramMap.put(AbstractGridFormat.READ_GRIDGEOMETRY2D.getName().toString(),new GridGeometry2D(new Rectangle(), new Rectangle()));

/* GridEnvelope2D gridEnvelope = new GridEnvelope2D(0, 0, mapDisplay.getWidth(), mapDisplay.getHeight());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the commented lines on intial commit? Are you kind to remove these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the bad code formatting. Hope everything is fine now :)

Signed-off-by: sloob <sebastian.loob@ibykus.de>
@fgdrf
Copy link
Contributor

fgdrf commented Dec 20, 2018

Looks good! Build-Job failure hasn't to do anything with your changes (https://ci.eclipse.org/udig/job/uDig-PR/38/)

Many thanks again for this improvement

@fgdrf fgdrf merged commit 5c9f993 into locationtech:master Dec 20, 2018
@sloob sloob deleted the faster_raster_rendering branch December 21, 2018 07:25
fgdrf pushed a commit to fgdrf/udig-platform that referenced this pull request Dec 23, 2019
…ontech#299)

* faster rendering by looking at min and max scales before read

Signed-off-by: sloob <sebastian.loob@ibykus.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants