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 webgl support to Annulus, Wedge and AnnularWedge #12704

Merged
merged 38 commits into from Apr 6, 2023

Conversation

mattpap
Copy link
Contributor

@mattpap mattpap commented Jan 1, 2023

  • AnnularWedge:
    image
  • Annulus:
    image
  • Wedge:
    image

This PR:

  • adds support for more GL glyphs (Annulus, Wedge, AnnularWedge)
  • simplifies loading of GL glyphs in base glyphs (greatly reduced code duplication)
  • greatly reduces code duplication and complexity in GL glyphs (in _set_data(), etc.)
  • improves handling of settings.force_webgl in tests

TODO:

  • resolve issues with GL attribute number limitations

@mattpap mattpap added this to the 3.1 milestone Jan 1, 2023
@codecov
Copy link

codecov bot commented Jan 1, 2023

Codecov Report

Merging #12704 (c8b0271) into branch-3.2 (0da7e85) will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           branch-3.2   #12704   +/-   ##
===========================================
  Coverage       92.35%   92.35%           
===========================================
  Files             315      315           
  Lines           19963    19963           
===========================================
  Hits            18437    18437           
  Misses           1526     1526           

Copy link
Member

@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

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

Noting that the new WebGL AnnularWedge and Wedge glyphs do not support line joins yet. It doesn't have to be in this PR as it only really affects large line widths, but it will need to be done eventually.

bokehjs/src/lib/models/glyphs/webgl/annulus.ts Outdated Show resolved Hide resolved
bokehjs/src/lib/models/glyphs/webgl/marker.frag Outdated Show resolved Hide resolved
#endif

#if defined(USE_ELLIPSE)
// From https://www.shadertoy.com/view/tttfzr (MIT licensed)
Copy link
Member

Choose a reason for hiding this comment

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

I am not really comfortable with copying someone else's code and pasting it into Bokeh. Ideally I would like the person submitting such code to demonstrate they understand it and take ownership of it in terms of supporting it in future, for two reasons:

  1. I'd like to ask about the iterative Newton solver in the code block below. Why iterative? Why 5 iterations? Does it cope well with long thin ellipses? Is the 0.01 value critical or would 0.0 be OK?
  2. Eventually the code for some of these new shapes is going to have to support line joins and line dashes, so it is going to need to be understood and modified.

Copy link
Member

Choose a reason for hiding this comment

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

@ianthomas23 it looks like this ellipse code has been removed for now, are you otherwise happy with this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure I commented earlier, but maybe I didn't click "Comment". In anycase, I removed the implementation of ellipse glyph, because it's the most complex of new glyphs, one that I don't fully understand and it's not urgently needed anyway. I may get back to this at some point, when I can actually explain the details of this implementation.

@mattpap mattpap modified the milestones: 3.1, 3.2 Feb 9, 2023
@mattpap mattpap changed the base branch from branch-3.1 to branch-3.2 March 4, 2023 15:48
@mattpap mattpap changed the title Add webgl support to Annulus, Wedge, AnnularWedge and Ellipse Add webgl support to Annulus, Wedge and AnnularWedge Mar 28, 2023
Copy link
Member

@ianthomas23 ianthomas23 left a comment

Choose a reason for hiding this comment

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

Thanks for removing the ellipse changes. As discussed, this is something we can return to later.

@mattpap
Copy link
Contributor Author

mattpap commented Apr 6, 2023

@ianthomas23, this is already approved, but I forgot to update release notes and documentation. This PR was rebased, but only the last two commits make any changes.

@ianthomas23
Copy link
Member

@mattpap Understood. That's OK by me, so feel free to self-merge if/when you are happy with CI.

@mattpap mattpap merged commit 5190f24 into branch-3.2 Apr 6, 2023
26 of 27 checks passed
@mattpap mattpap deleted the mattpap/gl_glyphs branch April 6, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants