[take 2] Implement scatter cliponaxis: false
#1861
Conversation
- keep track of which subplots have 1 or more `cliponaxis: false` traces + if so, don't clip the subplot layer + instead, clip all trace module layers except for 'scatterlayer' + when `cliponaxis: false`, clip all lines and errorbar groups - use ax.isWithinRange and Drawing.hideOutsideRangePoint to hide markers and text node that are out of range.
- similar to the cartesian case, keep track of subplots that have 1 or more `cliponaxis: false` traces - override setConvert's ax.isPtWithinRange - add relative clip path def (to appropriately clip lines group)
|
@alexcjohnson @geocosmite in 7228d5d, you'll notice that the marker and text node appear below the axis labels and ticks. After some thoughts, we're thinking about adding another attribute (maybe Arguably, trace layers should always be above axis labels and ticks, except for axes with |
src/components/drawing/index.js
Outdated
| var x = d.xp || xa.c2p(d.x), | ||
| y = d.yp || ya.c2p(d.y); | ||
| var x = d.xp = xa.c2p(d.x); | ||
| var y = d.yp = ya.c2p(d.y); |
There was a problem hiding this comment.
wow, || is 4 years old there... but it doesn't look to me like .xp and .yp are used anywhere except https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/plot.js#L489 which is a near copy of this routine. Take it out? Must have been a perf optimization that we did long ago but is now mostly gone.
src/components/drawing/index.js
Outdated
| drawing.hideOutsideRangePoint = function(d, sel, xa, ya) { | ||
| sel.attr( | ||
| 'visibility', | ||
| xa.isPtWithinRange(d) && ya.isPtWithinRange(d) ? null : 'hidden' |
There was a problem hiding this comment.
possibly better to use display: 'none' instead? visibility: 'hidden' still includes the element in the rendering pipeline (so children could be made visible for example, as we've seen!), which is probably slower and also means it still contributes to the bounding box (potential confusion for illustrator? I suppose we could also add something to our static plot pipeline to remove visibility: 'hidden' and display: 'none' elements completely...)
There was a problem hiding this comment.
possibly better to use display: 'none' instead?
Would be nice to benchmark these things, but you make a good argument for display. Good point above illustrator not liking visibility 👍
src/components/errorbars/plot.js
Outdated
| errorbars.call( | ||
| Drawing.setClipUrl, | ||
| plotinfo._hasClipOnAxisFalse ? plotinfo.clipId : null | ||
| ); |
There was a problem hiding this comment.
perhaps it would simplify things if when we calculate _hasClipOnAxisFalse we stash plotClipId and layerClipId (each of which is either clipId or null)?
Also a debatable style point, I've started avoiding .call when it's not part of a method chain - so this would just be Drawing.setClipUrl(errorbars, clipId)
src/plots/cartesian/index.js
Outdated
| plotinfo.plot.call(joinPlotLayers); | ||
|
|
||
| for(var i = 0; i < constants.layers.length; i++) { | ||
| joinLayer(plotinfo.plot, 'g', constants.layers[i]); |
| @@ -257,6 +285,9 @@ proto.adjustLayout = function(ternaryLayout, graphSize) { | |||
| _this.clipDef.select('path').attr('d', triangleClip); | |||
| _this.layers.plotbg.select('path').attr('d', triangleClip); | |||
|
|
|||
| var triangleClipRelative = 'M0,' + h + 'h' + w + 'l-' + (w / 2) + ',-' + h + 'Z'; | |||
| _this.clipDefRelative.select('path').attr('d', triangleClipRelative); | |||
There was a problem hiding this comment.
heh, yet another consequence of when we had nested <svg> elements instead of clip paths - I suspect we could simplify a whole lot of stuff by not translating containers, but that's a big project. Until then we need stuff like this 👍
There was a problem hiding this comment.
Thanks for tip. This simplified a lot of plot.js level logic. Done in 0496144
|
Feels great panning with these plots @etpinard - watching the points pop in and out of view! Two strange things I'm seeing right now while panning (both of which are fixed on mouseup, the issue is only during the pan interaction):
|
|
Some updates:
Now, some questions came up in
|
Just played with this a bit on browserstack: the chrome bug came in between versions 31 and 32. That's pretty ancient, If we decide not to work around it (which I guess I'd be OK with, it's fairly obscure), we should at least lock down when it happens and make sure it's on their radar to fix. That said, IE and Edge have a substantially worse problem with this type of panning, they don't seem to translate the clip path correctly: |
- `display: 'none'` completely removes the elements from the rendering pipeline as is likely faster is all browsers.
- to DRY up Drawing.setClipUrl calls downstream, where `if(plotinfo._hasClipOnAxisFalse)` statements are removed.
Lets step back and look at what these options do and why, and what we're trying to accomplish with the changes we're making now. The options to
But that rationale brings up another way to do this, something like |
Good idea. I can see for example wanting to layer the axis either above or below shapes, which are themselves above or below the data... Not sure about
Seems reasonable. If we're accidentally drawing axis lines somewhat underneath the data and giving them a thinned or ragged look, that could give us a subtly unpolished look for no apparent reason, whereas markers dropping underneath the axis are obvious and easy to fix - just make sure to mention |
|
Good call. Going with
Good call here too. Dropping those |
- which is more meaningul - other misc lint
- has 2 possible values: 'above traces' and 'below traces' for now, but opens the door for other values down the road.
- show off `(x|y)axis.layer` - lock down behavior for `cliponaxis: true` traces on hasClipOnAxisFalse subplot - add annotations for more descriptive mock
done in #1871 |
- which replaces all `require('d3')` with `require('./strict-d3')`
- so that strict-d3.js can more easily be used across all our tools
- activate when bundling plotly.js in jasmine tests,
`npm run watch` and `npm run cibuild`
- 🔪 <script> injecting old IIFE
- require('fast-isnumeric') in strict-d3.js
- instead of on <clipPath> element itself - see https://codepen.io/etpinard/pen/eRbxrp for more info on the problem - lock down this rule by adding condition in strict-d3.js
Implement `axis.layer` with 'above traces' and 'below traces' values
|
Just spent a bit of time playing with this in various browsers - I think you've nailed it! 💃 |
Ha. Yeah, I noticed the bug too, but I didn't know it had to do with the Track issue ➡️ #1879 |
|
Merging this thing. It's about time. But this leaves the item under MAYBEs unimplemented. See companion issue ➡️ #1880 |
Resolves #1385 (hopefully) and supersedes #1824
This PR implements
cliponaxis: falseby adding some logic around each subplot's<g.plot>layer. In brief, subplots that have 1 or morecliponaxis: falsetrace don't clip the<g.plot>layer (where all trace layers reside) and instead clip all trace module layers except<g.scatterlayer>separately. In<g.scatterlayer>,cliponaxis: falsetraces clip all line and error bars group while leaving marker and text node unclipped. Marker and text nodes visibility is then determined viaax.isWithinRangeand set to'hidden'when they fall outside the axis ranges. Note that, I chose to hide those outside range nodes instead of removing them like in #1824 to more easily handle panning (see 1909034)Ternary scatter
cliponaxis: falseis implement similar by mocking a few thing likeax.isWithinRangeand subplot options inscatterternary/plot.js. Note that I had to add another clip path definition (see 4b5c4bb). There might be a way to DRY this up 🌴MAYBEs (as in #1824):