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

bar: avoid line between stacked bars #4191

Closed
antoinerg opened this issue Sep 16, 2019 · 8 comments
Closed

bar: avoid line between stacked bars #4191

antoinerg opened this issue Sep 16, 2019 · 8 comments
Labels
bug something broken

Comments

@antoinerg
Copy link
Contributor

Because of antialiasing, we produce subpixel artifacts between stacked bars which appear as white lines. This is shown in this Codepen

This has been discussed in this Stack Overflow thread and in #4149. Because bars are rectangles, I wonder if we shouldn't set shape-rendering="crispEdges" on them 🤔

cc @alexcjohnson

@antoinerg antoinerg added bug something broken and removed bug something broken labels Sep 16, 2019
@alexcjohnson
Copy link
Collaborator

huh, why doesn't that example get crispEdges? Normal stacked bars DO, eg https://rreusser.github.io/plotly-mock-viewer/#bar_stackto1
Screen Shot 2019-09-16 at 6 27 25 PM

I seem to recall some cases where we thought it should be avoided, but I don't remember what those were. Anyway even if there are such cases, your example is not one of them, it should be crisp.

@alexcjohnson alexcjohnson added the bug something broken label Sep 16, 2019
@antoinerg
Copy link
Contributor Author

huh, why doesn't that example get crispEdges? Normal stacked bars DO, eg

Very nice! I see that it does the right thing: there:

// for gapless (either stacked or neighboring grouped) bars use
// crispEdges to turn off antialiasing so an artificial gap
// isn't introduced.
.each(function(d) {
if((fullLayout.barmode === 'stack' && barcount > 1) ||
(fullLayout.bargap === 0 &&
fullLayout.bargroupgap === 0 &&
!d[0].trace.marker.line.width)) {
d3.select(this).attr('shape-rendering', 'crispEdges');

Turns out that in my example the layout.bargap defaults to 0.2 because the barmode is group (see bar/layout_defaults.js for details). Anyway, setting the layout.bargap to zero fixed the issue.

I think this is satisfactory and I'm not sure this qualifies as a bug anymore. I would be OK with closing this issue if you agree of course.

@alexcjohnson
Copy link
Collaborator

Ah right, single, non-stacked bars with gaps are better with crisp off. But in this case, even though it's just one trace, there's still self-stacking. Ideal would be if we can figure out when there is self-stacking like this and have that effectively count as barcount > 1 (line 35 ^^) - I think it'd be worth trying this before closing, esp. since we've recently changed our algorithms to create more self-stacking - and we have px encouraging that use case.

Too bad browsers don't have separate x/y crisp modes, that would really be the ideal. One can sort of do this by precisely rounding the bar edges and disabling crispEdges, but that falls apart if the user zooms their browser.

@etpinard etpinard self-assigned this Sep 25, 2019
@etpinard
Copy link
Contributor

I think it'd be worth trying this before closing, esp. since we've recently changed our algorithms to create more self-stacking - and we have px encouraging that use case.

Correct, this became the default behaviour after #3680

@etpinard
Copy link
Contributor

Here are a few cases where we don't set crispEdges and arguably we should:

Case 1

Plotly.newPlot(gd, [{
  type: 'bar',
  y: [1, 2],
  marker: {color: 'blue'}
}, {
  type: 'bar',
  y: [2, 1],
  marker: {color: 'blue'}
}], {
  showlegend: false
})

looks like

image

Note that setting both trace to same marker.color makes the artifact easier to spot. Adding bargroup:0 to the layout gets us crispEdges.

Case 2

Plotly.newPlot(gd, [{
  type: 'bar',
  x: [0, 0],
  y: [1, 2],
}])

image

just a more minimal example to the one from the codepen in the issue header.

Case 3

Plotly.newPlot(gd, [{
  type: 'bar',
  x: [0, 0],
  y: [1, 1],
  base: [0, 1]
}], {
  barmode: 'overlay'
})

image



Now a few questions:

  • Should we try to turn crispEdges every time two bars touch?
  • Or should we be more selective, and turn on cripsEdges when things look really bad otherwise?
  • How much of a (perf) penalty do we pay by setting crispEdges?
  • Should we try to find alternatives to crispEdges similar to we what currently do here:

if(!gd._context.staticPlot) {
// if bars are not fully opaque or they have a line
// around them, round to integer pixels, mainly for
// safari so we prevent overlaps from its expansive
// pixelation. if the bars ARE fully opaque and have
// no line, expand to a full pixel to make sure we
// can see them
var op = Color.opacity(mc);
var fixpx = (op < 1 || lw > 0.01) ? roundWithLine : expandToVisible;
x0 = fixpx(x0, x1);
x1 = fixpx(x1, x0);
y0 = fixpx(y0, y1);
y1 = fixpx(y1, y0);
}

@etpinard
Copy link
Contributor

If we end up deciding to only fix the "self-stacking" case, stashing a hidden flag here:

if(base) bar.b = base;

and handling it here

if((fullLayout.barmode === 'stack' && barcount > 1) ||
(fullLayout.bargap === 0 &&
fullLayout.bargroupgap === 0 &&
!d[0].trace.marker.line.width)) {
d3.select(this).attr('shape-rendering', 'crispEdges');
}

should suffice.

@etpinard etpinard removed their assignment Sep 25, 2019
@alexcjohnson
Copy link
Collaborator

Should we try to turn crispEdges every time two bars touch?

That would be super tough to figure out

Or should we be more selective, and turn on cripsEdges when things look really bad otherwise?

I might even go the other way: what are the situations when crispEdges looks bad and we need to NOT enable it? Frankly the only one that occurs to me is narrow bars/gaps. If bars - OR the gaps between them - are, for example, somewhere between 2 and 3 px wide, crisp will make some of them 2px and others 3px, which looks junky, particularly if there are gaps. Even worse, if the bars or gaps are between 0 and 1 px, they can alternate between visible and completely gone.

How much of a (perf) penalty do we pay by setting crispEdges?

I'd assume the opposite, crispEdges disables antialiasing, and the extra blending it entails, so should be faster than leaving it off.

Should we try to find alternatives to crispEdges similar to we what currently do here:

Note that that solution fails with CSS transforms that scale the bars. Of course all hell breaks loose if you have a CSS rotation (that's not a multiple of 90 degrees)...

@gvwilson
Copy link
Contributor

Hi - we are currently trying to tidy up Plotly's public repositories to help us focus our efforts on things that will help users most. Since this issue has been sitting for several years, I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our backlog. Thanks for your help - @gvwilson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

No branches or pull requests

4 participants