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

[BUG] BoxAnnotation doesn't respect fill_color=None #9877

Closed
lucius-messerer opened this issue Apr 3, 2020 · 6 comments · Fixed by #9886
Closed

[BUG] BoxAnnotation doesn't respect fill_color=None #9877

lucius-messerer opened this issue Apr 3, 2020 · 6 comments · Fixed by #9886

Comments

@lucius-messerer
Copy link

@lucius-messerer lucius-messerer commented Apr 3, 2020

ALL software version info (bokeh, python, notebook, OS, browser, any other relevant packages)

Bokeh 2.0.0 | Python 3.7 | Firefox 74.0 | Windows 10 build 16299.1747

Description of expected behavior and the observed behavior

Expected behavior: Glyphs generally support turning off fill by supplying a fill_color value of None. Would expect the same from BoxAnnotation.

Observed behavior: BoxAnnotation (and maybe others? haven't tested) instead appears to have a white fill with a low (but nonzero) alpha under these conditions. Setting fill_alpha to zero has the desired effect, but this is not my standard usage pattern for turning off fill, and it should probably be consistent anyway.

Complete, minimal, self-contained example code that reproduces the issue

import bokeh.plotting
import bokeh.models

bokeh.plotting.output_file("box_annotation_test.html")

fig = bokeh.plotting.figure(width=500, height=500, x_range=[0, 3], y_range=[0,3])

fig.circle(x=[1, 1, 2, 2], y=[1, 2, 1, 2], size=100, line_color=None, fill_color="Red")

ba = bokeh.models.BoxAnnotation(bottom=1, top=2, left=1, right=2, line_color="Blue",
                                line_dash="dotted", fill_color=None)
fig.add_layout(ba)

bokeh.plotting.save(fig)

Screenshots or screencasts of the bug in action

bokeh_plot

The BoxAnnotation is drawn between the centers of the red circles. Notice the "notches" of paler red where the box intersects them.

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Apr 3, 2020

Right, it's currently not explicitly checking for null condition on the JS side, so it is rendering "null" color with a default opacity of 0.4 (whatever canvas defines that to do). For now you can add fill_alpha=0 as a workaround:

Screen Shot 2020-04-03 at 1 48 25 PM

Since it should be a simple fix and since there a a workaround I will leave it for now a Good First Issue

@Tekaichi
Copy link
Contributor

@Tekaichi Tekaichi commented Apr 6, 2020

We can then assume when fill_color = None, fill_alpha should be set to zero then?
If so, I'm opening a PR with this fix.

@mattpap
Copy link
Contributor

@mattpap mattpap commented Apr 6, 2020

We can then assume when fill_color = None, fill_alpha should be set to zero then?

No, fill_color == None means no rendering should happen. The value of fill_alpha in this situation is irrelevant.

@Tekaichi
Copy link
Contributor

@Tekaichi Tekaichi commented Apr 6, 2020

Both the lines and the background should not be rendered then or just the background (which happens if fill_alpha = 0)?

@mattpap
Copy link
Contributor

@mattpap mattpap commented Apr 6, 2020

Line and fill visuals should be considered independently, the same way as glyphs work. Essentially you have to replace:

this.visuals.fill.set_vectorize(ctx, i)
ctx.fill()

with

if (this.visuals.fill.doit) {
  this.visuals.fill.set_vectorize(ctx, i)
  ctx.fill()
}

and similarly change line visuals.

The tricky bit is that annotations have both canvas and CSS modes. This code only handles to former and you will have to figure out the later.

@Tekaichi
Copy link
Contributor

@Tekaichi Tekaichi commented Apr 6, 2020

Ok, thanks for the feedback! Fixing that on the PR!

bryevdv pushed a commit that referenced this issue Apr 8, 2020
#9886)

* Issue #9877, if fill_color = None, the box should not be rendered

* Removed unrelated ongoing code

* Removed unecessary spaces in visuals.ts. The file is now the same as the original

* Fixed fill_color = None behavior to match the expected one as mentioned in Issue#9886 by matt

* Removed unecessary spacing

* Fixes

* Version ts reversed

* Version.ts shouldnt be here

* Revert package-lock.json..

* Removed trailing spaces

* line_color = None -> line is not rendered
@mattpap mattpap removed this from the short-term milestone Apr 8, 2020
@mattpap mattpap added this to the 2.0.2 milestone Apr 8, 2020
mattpap added a commit that referenced this issue Apr 8, 2020
mattpap added a commit that referenced this issue Apr 8, 2020
mattpap added a commit that referenced this issue Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants