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.

Sign up for GitHub

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

fixed textpoint alignment #6967

Merged
merged 1 commit into from
Sep 22, 2024
Merged

fixed textpoint alignment #6967

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/typography/p5.Font.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,8 @@ p5.Font = class {
textToPoints(txt, x, y, fontSize, options) {
const xOriginal = x;
const result = [];

const p = this.parent;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@dhowe dhowe Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that makes sense to me. In the mean time, do you think this change is ok to merge?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dhowe Should we include the above as a default option, or do you think the alignment should be a required argument?

Copy link

@araid araid Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mathewpan2 mathewpan2 Apr 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

_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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

let pos;
let lines = txt.split(/\r?\n|\r|\n/g);
fontSize = fontSize || this.parent._renderer._textSize;

Expand Down Expand Up @@ -376,6 +377,14 @@ p5.Font = class {

for (let l = 0; l < pts.length; l++) {
pts[l].x += xoff;
pos = this._handleAlignment(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

p._renderer,
line,
pts[l].x,
pts[l].y
);
pts[l].x = pos.x;
pts[l].y = pos.y;
result.push(pts[l]);
}
}
Expand All @@ -386,7 +395,6 @@ p5.Font = class {

y = y + this.parent._renderer._textLeading;
}

return result;
}

Expand Down
Loading