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 n-gon glyph #14027

Merged
merged 6 commits into from
Aug 21, 2024
Merged

Add n-gon glyph #14027

merged 6 commits into from
Aug 21, 2024

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented Aug 15, 2024

This PR adds an Ngon glyph for regular n-sided polygons with data-space radii supplied. Additionally, a RadialGlyph was factored out of Circle to become the common base.

@ianthomas23 I do not know how much time you have to devote to Bokeh these days, but I wanted to at least check whether you would be interested and available to add WebGL support for this new glyph. I think it would be nice to have, but I simply do not have the expertise to take on the task.

@ianthomas23
Copy link
Member

@ianthomas23 I do not know how much time you have to devote to Bokeh these days, but I wanted to at least check whether you would be interested and available to add WebGL support for this new glyph. I think it would be nice to have, but I simply do not have the expertise to take on the task.

I can do that, in a separate PR.

@bryevdv bryevdv added this to the 3.6 milestone Aug 16, 2024
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.

I've tried this out locally, looks good to me.


x = NumberSpec(default=field("x"), help="""
The x-coordinates of the center of the n-gons.
""")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer all of these properties, except the only new one n, should be distributed in base classes, the same way as it is done in bokehjs. I don't see much value in having so much duplication just to change circle to n-gon; in this particular case, but this applies generally to models on the Python side. Having "The x-coordinates of the center of the glyph" is a perfectly sufficient description in my eyes.

Copy link
Member Author

Choose a reason for hiding this comment

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

ngon has some additional specific requirements to note, i.e. that the radius measures to a vertex, rather than, say, to the middle of an edge, and that a vertex is always located directly above the point, at angle=0.

Beyond that, while I definitely understand the motivation to "factor" things as much as possible for our sake, for users' it is better to actually be as specific as possible everywhere. So I am 👎 on making any changes to the way things are in the PR.

@bryevdv bryevdv requested a review from mattpap August 20, 2024 18:57
@bryevdv bryevdv merged commit 0296a2d into branch-3.6 Aug 21, 2024
22 checks passed
@bryevdv bryevdv deleted the bv/13905-ngon branch August 21, 2024 14:59
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.

[FEATURE] triangle relative size by data units
3 participants