Heatmap ordering bug #2917
Heatmap ordering bug #2917
Conversation
(all except scatter because of its animations)
|
Huh, on my machine as well as on CI |
since it's inside the contour group now it's unnecessary and in fact since I removed the uid-based class it was unused
| @@ -636,8 +636,6 @@ function clipGaps(plotGroup, plotinfo, clips, cd0, perimeter) { | |||
| else clipId = null; | |||
|
|
|||
| plotGroup.call(Drawing.setClipUrl, clipId); | |||
| plotinfo.plot.selectAll('.hm' + cd0.trace.uid) | |||
| .call(Drawing.setClipUrl, clipId); | |||
There was a problem hiding this comment.
Alright, GH is working again with these images, must've been an internal glitch last night.
The reason for the image diffs is that in master (right) we were applying the clip-path to both the heatmap group and the contour group... and now that the heatmap is inside the contour group that was redundant but apparently it was still having an effect on antialiasing. The Drawing.setClipUrl here that was supposed to be clipping the heatmap actually wasn't doing anything, since I removed the uid-based class from the heatmap.
So in fact the image diff in this PR probably nearly (or exactly) reverts the diff from contour-legend https://github.com/plotly/plotly.js/pull/2891/files?short_path=2b47456#diff-2b474566a55f847908961b61d6e0cba4
| traces.enter().append('g') | ||
| .attr('class', cls); | ||
|
|
||
| traces.each(function(cd) { |
There was a problem hiding this comment.
Ah very nice! That cls.replace(/s/g/, '.') is a nice touch.
I think makeTraceGroups should work with all svg-based subplot types, so perhaps we could move this to src/lib/? No sure where it would belong. There's lib/gup.js which has "general-update-pattern" helpers, maybe that would be a good fit.
Personally, I would not have included plotOneFn as an argument and here just called traces.order(), return traces and make _module.plot handle the trace.each "plot-one" logic. This way we wouldn't be tied down to one plotOneFn call signature and we wouldn't have to pass gd, plotinfo to makeTraceGroups, perhaps making things more reusable. But that's really more of a personal opinion, so consider this comment non-blocking 👌
There was a problem hiding this comment.
I was trying to match the existing pattern (from contourPlot.plotWrapper) but yeah, taking plotOne out would be cleaner and even more generalizable, I'll do that. Originally I wasn't returning the final selection but now that we have that this will be easy.
| * @param {array} calcdata: as in gd.calcdata (or a subset) | ||
| * @return {object} lookup object of uids (`uid: 1`) | ||
| */ | ||
| exports.getUidsFromCalcData = function(calcdata) { |
| @@ -636,8 +636,6 @@ function clipGaps(plotGroup, plotinfo, clips, cd0, perimeter) { | |||
| else clipId = null; | |||
|
|
|||
| plotGroup.call(Drawing.setClipUrl, clipId); | |||
| plotinfo.plot.selectAll('.hm' + cd0.trace.uid) | |||
|
Alright, more general pattern in 69dee75, and used it to 🌴 pie in 9ddad4c and fix choropleth e892923 and scattergeo 521d27f ( There are likely still similar hide/show ordering bugs in some other standalone svg trace types, eg |
src/traces/bar/plot.js
Outdated
|
|
||
| if(!plotinfo.isRangePlot) cd0.node3 = plotGroup; | ||
| var bartraces = Lib.makeTraceGroups(barLayer, cdbar, 'trace bars').each(function(cd) { |
There was a problem hiding this comment.
Ha yes. Lib.makeTraceGroups looks very similar to Lib.ensureSingle which have similar purpose. 👌
Wow. That's great! Thanks for adding test to scattergeo and choropleth 🔒 this down for good. I'll take care of
I'll take care of it before my fix-intermittent-gl-test-failure commit. Once again -> 💃 |
Fixes #2907 - dynamic ordering of heatmap and carpet traces
🔪 the now-obsolete
getUidsFromCalcDataBut then I realized this trace group management pattern is identical for all cartesian traces, so I made a single helper
makeTraceGroupsto handle this in all types, exceptscattersince animations give it somewhat different needs.cc @etpinard