Skip to content

Correctly handle odd-length dash patterns in WebGL #11819

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

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

ianthomas23
Copy link
Member

Fixes item 6 of issue #11050.

Line dash patterns with an odd length were not handled correctly in the WebGL output backend, this PR fixes that. Correct behaviour to match the canvas backend is to repeat the dash pattern so that its length is even. Fix motivated by comment from @mattpap #10861 (comment).

Dash patterns coming from Python are already sanitised before they reach the WebGL code, so this is only an issue for direct use of BokehJS.

I have modified an existing visual test that was using a dash pattern of [4, 4] to now use [4]. The PNG output is therefore the same as before, but is obtained by a slightly different code path. So strictly speaking this is not a full test in that it does not fail before the fix. I am happy to add an explicit full test if that is preferred.

if (pattern.length % 2 == 1)
pattern = pattern.slice(0, -1)
pattern = concat([pattern, pattern])
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering why this is necessary altogether, but visuals/line does only partial normalization and lets canvas to do final normalization.

@mattpap mattpap merged commit 7b85b04 into branch-3.0 Nov 18, 2021
@mattpap mattpap deleted the ianthomas23/webgl_odd_length_dash_patterns branch November 18, 2021 15:47
@mattpap
Copy link
Contributor

mattpap commented Nov 18, 2021

btw. @ianthomas23, on PRs that don't close issues, there should be full set of labels.

@ianthomas23
Copy link
Member Author

btw. @ianthomas23, on PRs that don't close issues, there should be full set of labels.

I do seem to have a mental block about labels. It should be within my mental capacity...

@ianthomas23
Copy link
Member Author

Clarifying the situation here for future reference following discussion with @mattpap.

  • Bokeh python does not alter the dash patterns, I was wrong in stating this earlier. This is a good thing.
  • It might be better to push the handling of odd-length patterns out of the WebGL code and up to core/visuals/line.ts, for example in resolve_line_dash.

@ianthomas23 ianthomas23 modified the milestones: 3.0, 2.4.3 May 13, 2022
ianthomas23 added a commit that referenced this pull request May 13, 2022
* Use weekly channel for gmap plots (#12113)

* Add DatetimeRangeSlider (#12034)

* Add DatetimeRangeSlider

* Add tests

* Add docs

* Simplify EqHistColorMapper calculations (#12084)

* Simplify EqHistColorMapper calculations

* Add numerical unit tests

* Better numerical unit tests

* Support rescale_discrete_levels in EqHistColorMapper (#12121)

* Drop baseline testing on MacOS and Windows (part of #11906)

* Try more of #11906

* Pin to django 3.x (#11867)

* Fix new typings failures (#11971)

* 12093 Update union syntax (#12094)

* Update union syntax under typings directory

* Update Sequence and Callable imports

* Force webgl backend when requested for testing (#11823)

* Force webgl backend when requested for testing

webgl backend normally falls back to canvas rendering, but that's
undesirable when testing specifically webgl support. For testing
the fallback mechanism, use `settings.force_webgl = false`.

* Install a specific version of chromium on linux

* Python 3.7 specific mypy fixes

* isort fix

* remove unused ignores

* more mypy

* yet more mypy

* re-skip previously skipped SVG test

* Clarify RenderRoot for components (#12036)

* Clarify RenderRoot for components

* unrelated typo fix

* Fix color function call (#11751)

* Fix typo in git.py (#12106)

verison -> version

* Fixed typo: s/server/serve/ (#12051)

* Add docs_toc block (#11989)

* Fixed xwheel and xzoom tools with hard bounds (#11832) (#11834)

* Change DatetimeRangeSlider tests to fit older testing framework

* Fix webgl dashed line joins with butt end caps (#11814)

* Fix webgl dashed line joins with butt end caps

* Update baseline images

* Correctly handle odd-length dash patterns in webgl (#11819)

* Correctly render straight webgl lines with bevel joins (#12065)

* Limit tool-related tap events to the frame (#11938)

* 11965 fixes that null value with unspecified nan_format crashes table rendering (#12098)

* #11965: fixed error when trying to show null values in table columns using ScientificFormatter

* #11965: changed string to double quotes

* Squashed commits:

[4355e9165] 11965: removed whitespaces (+1 squashed commits)

Squashed commits:

[46984940d] #11965: moved nan_format to StringFormatter and made non-nullable

* #11965: changed default nan_format to NaN

* #11965: default nan/null format for all string derived cellformatters set to -

Co-authored-by: Harm Buisman <h.buisman@iknl.nl>

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>
Co-authored-by: Jada Lilleboe <82007190+jadalilleboe@users.noreply.github.com>
Co-authored-by: Mateusz Paprocki <mattpap@gmail.com>
Co-authored-by: g-parki <61096711+g-parki@users.noreply.github.com>
Co-authored-by: Ikko Ashimine <eltociear@gmail.com>
Co-authored-by: Rick van Hattem <wolph@wol.ph>
Co-authored-by: Timo Cornelius Metzger <39711796+tcmetzger@users.noreply.github.com>
Co-authored-by: Florent <florentbr@gmail.com>
Co-authored-by: harmbuisman <harmbuisman@gmail.com>
Co-authored-by: Harm Buisman <h.buisman@iknl.nl>
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.

2 participants