Add n-gon glyph #14027
Add n-gon glyph #14027
Conversation
I can do that, in a separate PR. |
ianthomas23
left a comment
ianthomas23
left a comment
There was a problem hiding this comment.
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.
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.
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 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. |
This PR adds an
Ngonglyph for regular n-sided polygons with data-space radii supplied. Additionally, aRadialGlyphwas factored out ofCircleto 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.