[Rendering] Only render in preview jobs layers that are fast enough #5789
Conversation
nyalldawson
force-pushed
the
render_jobs
branch
from
0063169
to
73d82ef
Compare
|
Nice. How does it deal with XYZ layers that aren't CPU intensive but might be slow due to connection speed to tile server? Will those also be discarded? |
|
Yes. But the virtual QgsDataProvider::renderInPreview method could be overrided by individual providers to add their own logic which decides whether the layer should be rendered. So XYZ layers could be treated specially and exempt from the time based check. |
|
@nyalldawson , excellent. |
|
Nice to see this back.
Can you remove the "could" in this sentence for tiled sources before merging? |
src/core/qgsmaprendererjob.h
Outdated
| * Returns the render time (in ms) per layer, by layer ID. | ||
| * \since QGIS 3.0 | ||
| */ | ||
| QMap< QString, int > layerRenderingTime() const { return mLayerRenderingTime; } |
There was a problem hiding this comment.
wouldn't perLayerRenderingTime() better reflect that this is not just the time for a single layer. Or layersRenderingTime() ?
There was a problem hiding this comment.
I'll change it to perLayerRenderingTime()
nyalldawson
force-pushed
the
render_jobs
branch
from
73d82ef
to
814191f
Compare
Done |
|
@nyalldawson , just compiled and tried this branch, it's a significant improvement, thanks. |
|
@nyalldawson , one thing: IMHO, we should render those preview tiles with an opacity < 100%, to clearly indicate those are preview tiles. Now that only a fraction of a project layers are rendered in the preview mode, dragging around can have weird-ish results (for e.g. a hillshade layer that doesn't render in preview tiles). Rendering the preview in say opacity 60% might actually be a nicer UX. |
Flagged as out-of-scope ;) |
|
@nyalldawson , it's a 2-line patch to the QgsMapCanvasItem::paint() function, and look at how much nicer the experience is: https://www.dropbox.com/s/hy9kxfz4a7we800/opacity.mp4?dl=0. OK to process forward with this? I'll push a PR. |
|
Got a "before" video for comparison? |
|
@nyalldawson here: https://www.dropbox.com/s/2cirisb4an64i2o/noopacity.mp4?dl=0 IMHO, it's an improvement. It's a good indication that preview tiles aren't a complete rendering (i.e. until now missing at the very least labels, and thanks to your work CPU-heavy raster/vector datasets). |
|
I'm 50/50 on it. Let's move it to a separate PR for discussion. |
|
@wonder-sk Are you happy with this approach? |
|
@nyalldawson , sounds good. |
|
Looks good to me - thanks! Maybe if the public API would use pointers to QgsMapLayer instead of layer IDs (to get rid of layer IDs from public API) and use QHash instead of QMap, but neither of those is important... |
This implements the improvements discussed in the mailing list thread https://lists.osgeo.org/pipermail/qgis-developer/2017-November/050524.html to avoid rendering layers in preview jobs that take too much time to render.
Only start the next job when the previous one has completely finished. Avoids flooding connection pools with too many quick requests.
nyalldawson
force-pushed
the
render_jobs
branch
from
814191f
to
38cca6c
Compare
Continues the work started by @rouault in #5645, addressing the comments from that thread.