-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add n-gon glyph #14027
Conversation
I can do that, in a separate PR. |
There was a problem hiding this 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. | ||
""") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This PR adds an
Ngon
glyph for regular n-sided polygons with data-space radii supplied. Additionally, aRadialGlyph
was factored out ofCircle
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.