Skip to content

Fix heatmap rendering when zsmooth=fast - #6565

Merged
archmoj merged 7 commits into
plotly:masterfrom
lvlte:heatmap_fast_zsmooth_fix
Apr 17, 2023
Merged

Fix heatmap rendering when zsmooth=fast#6565
archmoj merged 7 commits into
plotly:masterfrom
lvlte:heatmap_fast_zsmooth_fix

Conversation

@lvlte

@lvlte lvlte commented Apr 12, 2023

Copy link
Copy Markdown
Contributor

Fixes #6564

All details are in the issue description. I will add a markdown log file tomorrow.

@archmoj archmoj added bug something broken community community contribution status: reviewable labels Apr 13, 2023
@archmoj

archmoj commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Thanks for the PR.
To compare here is the updated codepen based on your PR:
After vs Before.

@archmoj

archmoj commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

To lock down the issue, please add a jasmine or image test.
You could use this simplified codepen.

@lvlte

lvlte commented Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

Ok, I will try to cover both the edge case (when the bug can be visualized) and the other case (invisible, which affects performance).

Do you know by the way why the build-and-test checks failed ? It seems to fail randomly on "webgl-jasmine" or "flaky-no-gl-jasmine" from what I saw (one succeeds, the other fails) but unless I missed something it should not be related to this PR.

@archmoj

archmoj commented Apr 13, 2023

Copy link
Copy Markdown
Contributor

Thanks. Those failures are common and are not related to your PR. I will rerun those two tests for you.

@lvlte

lvlte commented Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

Ok thank you.

To better illustrate what I mean by "edge case" (visual bug) vs "other case" (performance issue) : most of the time (when m*n is less than imageWidth*imageHeight) there will be a lot of dead pixels drawn outside the canvas, hence this is invisible but impacts performance :

To "see" it, on current master :

// if(zsmooth === 'fast') {
//     canvasW = n;
//     canvasH = m;
// } else {
//     canvasW = imageWidth;
//     canvasH = imageHeight;
// }
canvasW = imageWidth;
canvasH = imageHeight;

And with return [0, 0, 0, 1]; instead of return [0, 0, 0, 0]; in setColor,

Inspecting the final image of "zsmooth_methods" mock reveals how many (181x3 instead of just 5x3) pixels were drawn :

InkedScreenshot 2023-04-13 192322

In fact I was trying to find a way to improve heatmap rendering performances both with and without zsmooth and I land on this bug :).

Comment thread draftlogs/6565_fix.md Outdated
@archmoj

archmoj commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

Currently we have a bug in our image test system.
To avoid that please rename your new mock & baseline png (as well as in the jasmine require) from
heatmap_small_layout_zsmooth_fast to ``zz-heatmap_small_layout_zsmooth_fast`.
That way the order of baseline creation won't change in parallel runs and we should be good to go.
Thank you!

@archmoj

archmoj commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

Thanks very much for the great fix and tests! 🥇
💃

@archmoj
archmoj merged commit 3366e95 into plotly:master Apr 17, 2023
@lvlte

lvlte commented Apr 17, 2023

Copy link
Copy Markdown
Contributor Author

Thank you for your responsiveness !

@lvlte
lvlte deleted the heatmap_fast_zsmooth_fix branch August 20, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken community community contribution

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heatmap truncated when zsmooth="fast"

2 participants