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

Implement text wrapping #11

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vincentfretin
Copy link
Collaborator

I create the PR in draft with just the changes related to text wrapping to gather feedback if you have any.
Capture d’écran du 2022-12-20 17-52-02

I based my getLines function from https://stackoverflow.com/a/16599668

} else {
const lines = getLines(context, string, maxWidth);
lines.forEach(function(line, i) {
context.fillText( line, x, y + parseFloat( style.fontSize ) * 0.1 + i * parseFloat( style.fontSize ) * 1.3 );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I actually have no idea why I need 1.3 here. I also don't understand why we have the 0.1 here. Is this related to line-height maybe? 0.1 above, 0.1 below + 0.1 between lines?

Copy link
Owner

Choose a reason for hiding this comment

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

It's to vertically centre the text

@@ -259,8 +286,11 @@ function html2canvas( element ) {
y = rect.top - offset.top - 0.5;
width = rect.width;
height = rect.height;
// On Quest the font used to draw on canvas is bigger than on
// the desktop, compensate for this.
const maxWidth = width * 1.01; // 1.005 is good, but use 1.01 to be sure
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one is hacky. I didn't have the same result on Meta browser and Chrome desktop.
Also without this, the label for radio weren't in one line but on two lines.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't love it but whatever works tbh

@vincentfretin
Copy link
Collaborator Author

https://github.com/niklasvh/html2canvas project is using Intl.Segmenter (works on Chrome, not Firefox) to split the text into words, see
https://github.com/niklasvh/html2canvas/blob/6020386bbeed60ad68e675fdcaa6220e292fd35a/src/css/layout/text.ts#L105-L120 and there is a fallback function if it's not supported.
That would be better if you render Japanese.

@jrjdavidson
Copy link

I had a closer look and can't reproduce the error I mentioned in mrdoob/three.js#24781 so as far as I'm concerned this is the way to go.

@vincentfretin
Copy link
Collaborator Author

Great, thanks for testing.

Elettrotecnica added a commit to Elettrotecnica/aframe-vr that referenced this pull request Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants