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

Raster - paletted: fix slow rendering with huge number of classes #56784

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Mar 9, 2024

Fix #56652

Alternate fix for #56652, see #56677

Test run without this PR with 50K classes:
24.548s

With this PR:
1.599s

The (insane) test case of #56652 produces a noticeable freeze of about 2 seconds on my machine.

@elpaso elpaso added Regression Something which used to work, but doesn't anymore Bug Either a bug report, or a bug fix. Let's hope for the latter! backport queued_ltr_backports Queued Backports backport release-3_36 labels Mar 9, 2024
@github-actions github-actions bot added this to the 3.38.0 milestone Mar 9, 2024
Copy link

github-actions bot commented Mar 9, 2024

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit e2ea46f)

@nyalldawson
Copy link
Collaborator

Thanks @elpaso ! I wonder if we shouldn't still skip the whole logic in extreme circumstances. Is there any harm in doing so? A 1.5 second delay would still be nice to avoid if there's no harm in skipping this

@elpaso
Copy link
Contributor Author

elpaso commented Mar 11, 2024

Thanks @elpaso ! I wonder if we shouldn't still skip the whole logic in extreme circumstances. Is there any harm in doing so?

Yes, that would break RAT on-the-fly reclassification (but see later for the missing uses case).

A 1.5 second delay would still be nice to avoid if there's no harm in skipping this

I honestly do not think this is an issue, there is a much longer (~30 seconds) freeze when creating 50K classes before entering the renderer but the more important question is: what's the use case for a paletted renderer on a continuous floating point raster that would yield 50K classes?

I think we shouldn't change the logic here, if you want you can add a safeguard at the GUI level before creating the renderer that would warn the user about the potential nonsense of creating a paletted renderer with such a huge number of classes.

If there is a use case for that (which I doubt) then collapsing classes is probably something we would like to do in that case.

@elpaso
Copy link
Contributor Author

elpaso commented Mar 22, 2024

It's a pity this didn't make into today's release.

Let's hope for the next.

@elpaso elpaso merged commit 810a61a into qgis:master Apr 4, 2024
44 checks passed
@elpaso elpaso deleted the bugfix-gh56652-speed-regression-with-insane-paletted-classes branch April 4, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport queued_ltr_backports Queued Backports backport release-3_36 Bug Either a bug report, or a bug fix. Let's hope for the latter! Regression Something which used to work, but doesn't anymore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Paletted/Unique values" raster rendering is extremely slow using QGIS 3.34.4 / QGIS 3.36.0
2 participants