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

Add support for HSpan, VSpan, HStrip and VStrip #12677

Merged
merged 25 commits into from
Apr 24, 2023
Merged

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Dec 12, 2022

This adds support for span and band glyphs (infinite width or height depending on orientation). This is equivalent to Span and Band annotations, just supports everything that glyphs support (selection, inspection, webgl, etc.).

/cc @jlstevens @jbednar

TODO:

  • efficient hit testing (use some form of spatial index)
  • bounds (in one dimension)

image

Screencast_00001.mp4

@bryevdv
Copy link
Member

bryevdv commented Dec 12, 2022

Can we get a discussion opened for this, to actually go over and gain agreement about goals and requirements are?

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #12677 (d20917c) into branch-3.2 (92f4c42) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@              Coverage Diff               @@
##           branch-3.2   #12677      +/-   ##
==============================================
+ Coverage       92.39%   92.40%   +0.01%     
==============================================
  Files             315      315              
  Lines           19981    20023      +42     
==============================================
+ Hits            18461    18503      +42     
  Misses           1520     1520              

@mattpap mattpap force-pushed the mattpap/hv_span_band branch 2 times, most recently from 1541c24 to 2a12f1d Compare December 13, 2022 11:45
@jlstevens
Copy link
Contributor

jlstevens commented Dec 13, 2022

Looks great!

Efficient hit testing is something I am expecting to make use of later (but isn't a big priority yet).

@mattpap
Copy link
Contributor Author

mattpap commented Dec 13, 2022

@ianthomas23, how far are we from supporting webgl in MultiLine glyph? Or perhaps I could use LineGL here with NaN separators? (I can't due to scalar visuals in Line glyph). Also, when trying to make webgl initialization more robust (and fail in tests if glyphs don't provide webgl implementation), I got this test failing:

✗ Color support
 └── should combine alpha for line, fill and hatch on all output backends
Error: MultiLineView(j14953) does not support webgl

Fortunately that's the only issue discovered, expect for the test I added in this PR, which prompted stricter init in tests.

@ianthomas23
Copy link
Member

Can we get a discussion opened for this, to actually go over and gain agreement about goals and requirements are?

#12487 is the discussion I started about this, and is the natural place for the remaining necessary discussion.

@ianthomas23
Copy link
Member

@ianthomas23, how far are we from supporting webgl in MultiLine glyph? Or perhaps I could use LineGL here with NaN separators? (I can't due to scalar visuals in Line glyph).

It is unfortunately non-trivial to implement MultiLine glyphs in WebGL. It requires blits of the WebGL canvas to HTML canvas within a single glyph.render() call, which needs a redesign of the rendering pipeline. Using LineGL with NaNs is a good approach now if that works for you.

@bryevdv
Copy link
Member

bryevdv commented Dec 13, 2022

Thanks for the reminder @ianthomas23 I have left another comment. Just to be clear about my position: I am opposed to adding anything new until there is at least a sketch of a high level plan for "vectorized annotations". Adding things ad-hoc is how we got to where we are.

@mattpap mattpap force-pushed the mattpap/hv_span_band branch from 2a12f1d to 8ca3b8f Compare January 17, 2023 18:20
@mattpap mattpap force-pushed the mattpap/hv_span_band branch from 8ca3b8f to ede39f8 Compare January 31, 2023 00:02
@mattpap
Copy link
Contributor Author

mattpap commented Feb 8, 2023

VBand and HBand in this PR are indeed quite different from Band annotation. I think the best approach will be to come up with a different name. Maybe let's rename VBand and HBand to VStrip and HStrip. According to https://encyclopediaofmath.org/wiki/Strip (I'm not sure how reputable the source is), a "strip" is "the set of points in a plane between two parallel straight lines in this plane", which seems fitting. @bokeh/core any suggestions?

@jlstevens
Copy link
Contributor

jlstevens commented Feb 8, 2023

HStrip/VStrip seems perfectly reasonable to me!

@bryevdv
Copy link
Member

bryevdv commented Feb 8, 2023

I'm OK with the proposed new names. (I can't think of anything better).

I'd just like to reiterate that we still lack (that I am aware of) any documented plan or roadmap for annotations, that expresses any agreed-upon set of requirements or definitions (from multiple POVs) for what exactly annotations should "be" in a consistent way (a specific goal for the future), and I think that is very unfortunate.

@jbednar
Copy link
Contributor

jbednar commented Feb 9, 2023

My (perhaps naive) hope is for Bokeh annotations to differ from regular glyphs only in the default values of certain attributes they have. E.g. annotations by default don't participate in auto-ranging, and similarly for any other way annotations differ from glyphs. That way they would behave just as they always have, but users could decide for themselves if their particular use case needs particular behavior. That's just a wish from an end user, though, and I haven't looked at the code involved.

@jlstevens
Copy link
Contributor

jlstevens commented Feb 9, 2023

In addition to Jim just said, maybe 'annotation' is just a suggestion to the user that this type of glyph may be useful for putting other data in context. For instance, in addition to maybe disabling auto-ranging by default, other defaults might be set to make the annotation useful when put on top of other data.

In the case of VStrip/HStrip that might mean lowering the alpha so you can see whatever is under the strip. As VSpan and HSpan are simply lines, they can already be useful on top of other data without needing to use alpha.

@ianthomas23
Copy link
Member

I'd just like to reiterate that we still lack (that I am aware of) any documented plan or roadmap for annotations, that expresses any agreed-upon set of requirements or definitions (from multiple POVs) for what exactly annotations should "be" in a consistent way (a specific goal for the future), and I think that is very unfortunate.

I'd like to emphasise this ^^^

I started a discussion on this topic (#12487) almost 4 months ago, and only Bryan and myself have contributed to that discussion. There have been multiple requests for this in this PR but that discussion has not been added to. I cannot imagine that this PR will be reviewed and merged until that discussion includes something concrete about plans and roadmap. The easiest and quickest route for this PR to be accepted is therefore for @mattpap to do this as soon as possible.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 9, 2023

I started a discussion on this topic (#12487) almost 4 months ago, and only Bryan and myself have contributed to that discussion. There have been multiple requests for this in this PR but that discussion has not been added to. I cannot imagine that this PR will be reviewed and merged until that discussion includes something concrete about plans and roadmap. The easiest and quickest route for this PR to be accepted is therefore for @mattpap to do this as soon as possible.

The only reason we are discussing annotations in this PR, is due to a name conflict with an existing annotation, because of my misunderstanding of Band annotation. With a new name in place, I don't see the fate of this PR being bound to any decisions related to the future of vectorized annotations.

So far I didn't participate in the discussion, because I didn't have enough time to do the groundwork to understand whether the existing features of vectorized annotations (e.g. screen positioning) make sense in the context of glyphs. When I will do that, then I can come up with a plan. However, there were other priorities recently.

@philippjfr
Copy link
Contributor

Just my two cents, while from a technical perspective these two glyphs aren't what we have come to refer to as annotations in Bokeh, at a conceptual level they very much are likely to be used as annotations. So I think working out what the future of glyphs and annotations in bokeh is important and we should think about how VStrip and HStrip fit into whatever conceptual framework we come up with.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 9, 2023

(...) at a conceptual level they very much are likely to be used as annotations.

I'm not sure what that actually means, especially in the context of the requirements for this feature. What was requested, as far as I can tell (there is no feature request for this), was a graphical entity similar to LRTB glyphs, just infinite in one of the dimensions and:

  1. Allowing for large datasets.
  2. Supporting hit testing/hover tool.

Those two points are two defining characteristics of glyphs in bokeh (the second is a transitive characteristic, a consequence of fully supporting of being data driven, but nevertheless). The only defining characteristic of vectorized annotations is screen space positioning. Maybe one could consider the lack of auto-ranging as such, but vectorized annotations don't support auto-ranging, because it requires glyph-related logic that when implemented, would make them equivalent to glyphs.

So, how is feature going to be used?

@bryevdv
Copy link
Member

bryevdv commented Feb 9, 2023

Thanks for the comments everyone. As a result of them, I actually have a plan proposal in my head (that I think everyone will be able to get behind). I would like to get it down on paper in a thorough fashion, though, and it will take me a day or two to get to that. I will add it to #12487 and we can pick things up there. I will also try to close / consolidate some old issues that seem duplicative at this point.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 9, 2023

We had chance to discuss this PR in an internal bokeh meeting and we are inclined to postpone it for bokeh 3.2 under assumptions that this work will not be usable anyway until panel 1.0 and holoviews are released, and that we will follow up with bokeh 3.2 maybe in a month a after 3.1, which would get us back (give or take) on track with the original schedule for 3.x releases. However, @jbednar and @jlstevens did not participate in this discussion, so let us know what you think about this.

@jlstevens
Copy link
Contributor

... and that we will follow up with bokeh 3.2 maybe in a month a after 3.1 ...

If this is a realistic timeline for 3.2 then I think postponing till then is fine (given that 3.1 is imminent).

@mattpap
Copy link
Contributor Author

mattpap commented Feb 9, 2023

If this is a realistic timeline for 3.2 (...)

I think it's realistic. I have a few things going on as work in progress (e.g. canvas layouts, i.e multiple legend and color bar layout support, and detached legend and color bars), so we can have a smaller yet feature full release. We are also anticipating more regressions coming from panel and holoviews, so another release fairly soon after 3.1 will be probably inevitable. That can be a patch release, but it can a minor release as well.

@mattpap mattpap force-pushed the mattpap/hv_span_band branch from b6d85e6 to b1c56d3 Compare April 24, 2023 11:26
Copy link
Member

@bryevdv bryevdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@mattpap mattpap force-pushed the mattpap/hv_span_band branch from b1c56d3 to d20917c Compare April 24, 2023 15:19
@mattpap mattpap merged commit 9e941dd into branch-3.2 Apr 24, 2023
@mattpap mattpap deleted the mattpap/hv_span_band branch April 24, 2023 16:25
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* Add support for {H,V}{Span,Band}

* Add AuxHatch to bokehjs' glyph API

* Add support for GL rendering

* Don't imply ordering of band edges

* Update visual baseline with webgl output

* Add support for hit testing

* Rename {H,V}Band -> {H,V}Strip

* Update visual baselines after rebase

* Finalize implementation of hit testing

* Update LRTB webgl interface after rebase

* Update tests and visual baselines

* Add a rudimentary 1d spatial index

* Add preliminary unit tests

* Use spatial index and respect line widths

* Add release notes

* Add glyph API methods and tests

* Fix order of imports in glyphs/index.ts

* Use `bokeh-pull` in release notes

* Add min(u: Uniform) and unit tests

* Refactor long code lines

* Don't use inline magic constants

* Add support for generic legends

* Add plotting/spans_and_strips.py example

* Add reference examples

* Add documentation to user_guide/basic.rst
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants