fixed textpoint alignment #6967
Conversation
| @@ -344,7 +344,8 @@ p5.Font = class { | |||
| textToPoints(txt, x, y, fontSize, options) { | |||
| const xOriginal = x; | |||
| const result = []; | |||
|
|
|||
| const p = this.parent; | |||
There was a problem hiding this comment.
I think this means that it will use the alignment of the main canvas even when one is using the font to draw points on a p5.Graphics. That could be fine, since I'm not sure there's any better way to determine where you're going to use the points after you call this function. @dhowe what do you think?
There was a problem hiding this comment.
I think this means that it will use the alignment of the main canvas even when one is using the font to draw points on a
p5.Graphics. That could be fine, since I'm not sure there's any better way to determine where you're going to use the points after you call this function. @dhowe what do you think?
I don't love this, but it does highlight a bigger problem with the class, specifically that it cannot be used outside of p5 at present, which is something I think (?) we want to support (see #6830). My initial thinking for this class was that it was font-specific but rendering-agnostic and thus could be used with/w'out p5.
So my feeling is that it should probably be refactored only to access the p5.renderer if parameters are not otherwise specified AND the renderer actually exists, otherwise it should use sensible defaults. Obviously not a big priority as its been like this for awhile, but would be nice for 2.0 (and perhaps integrated into #6830)
There was a problem hiding this comment.
I think that makes sense to me. In the mean time, do you think this change is ok to merge?
There was a problem hiding this comment.
I'm thinking that perhaps we should include alignment as an option via the options argument? For cases like drawing to an offscreen buffer with different alignment than the main canvas
There was a problem hiding this comment.
@dhowe Should we include the above as a default option, or do you think the alignment should be a required argument?
There was a problem hiding this comment.
I think for now it's okay to expect users to set textAlignment in the main p5 canvas before calling textToPoints. If we wanted to pass alignment as a parameter, we'd also have to pass the values from textLeading, textWrap, not sure if any others (maybe textStyle?).
If it needs to be added, I'd rather move textSize and and all the other text... values to the options object than keep adding additional parameters.
There was a problem hiding this comment.
This was the intention, to use the options argument, not to change the function signature, so yes, I guess I'd argue for supporting all the related parameters in that object. The other option would be to only support these via the main renderer options, but this a) creates some potentially awkward code when drawing to an offscreen buffer, and b) doesn't allow for the class to be used outside of p5
There was a problem hiding this comment.
_handleAlignment itself is written as expecting a renderer to calculate the alignment. I guess to me it just feels weird to allow the user to change the alignment without using the renderer, since the function TextBounds() in the same class similarly uses the parent's renderer to calculate its own alignment. A solution could be to handle the alignment without using _handeAlignment, but I'm not sure if that would be the best solution.
There was a problem hiding this comment.
I think a key question is whether we want this class to be usable outside of p5, as discussed in #6830 (I would argue that we do, but that's just me)... If so there are easy answers to many of the questions raised above; if not, then we only need to think about cases when drawing to something other than the main canvas (likely not a very common use-case). @limzykenneth @Qianqianye ?
There was a problem hiding this comment.
I think even the case of the text points not being used directly in the canvas but rather sent as data to a server that can do other things with it (eg. control 3D printer or other computational manufacturing process) should probably be considered still. The more independent we can make each module the better I think, regardless of whether we end up making it usable outside of p5/browser environment.
| @@ -376,6 +377,14 @@ p5.Font = class { | |||
|
|
|||
| for (let l = 0; l < pts.length; l++) { | |||
| pts[l].x += xoff; | |||
| pos = this._handleAlignment( | |||
|
@mathewpan2 @dhowe @davepagurek Just want to check in, is this ready to merge? We can revisit this as part of 2.0 but want to get fixes in for 1.x where possible and not breaking changes. |
|
I think this is ok to merge for now, with a note that this entire class (p5.Font) needs to be rethought for v2.0, where, assuming these function are not moved to a separate module or library, we should support use with and without the renderer in p5, and also, ideally, outside of p5. |
|
Can we merge this, or is there a plan in place for 2.0? |
|
this can be merged (and there is a plan in place for 2.0) |
Resolves #6893
Changes:
Added the
_handleAlignmentfunction inp5.Font.jstotextToPoints, so that it also follows the renderer's current alignment.Screenshots of the change:
When
textAlign(CENTER, BASELINE);was called, the text points and the normal text would be misaligned.Before the change:
After the change:
PR Checklist
npm run lintpasses