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

Implement wrapWord parameter (#268) #345

Draft
wants to merge 8 commits into
base: dev
Choose a base branch
from
Draft

Implement wrapWord parameter (#268) #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
da89c2e
Add wrapWord option, no implementation yet
marcel-danilewicz-consult-red Jul 12, 2024
98b45c9
Implement wrapWord for sdf text renderer
marcel-danilewicz-consult-red Jul 17, 2024
8a939ad
Change wrapWord to string literal
marcel-danilewicz-consult-red Jul 26, 2024
4f47680
Implement wrapWord for canvas text renderer
marcel-danilewicz-consult-red Jul 29, 2024
c7f8c9c
Add wrapWord parameter to text-contain example
marcel-danilewicz-consult-red Jul 29, 2024
856cc7c
Improve breaking algorithm
marcel-danilewicz-consult-red Jul 30, 2024
3c98b21
Change order of conditions
marcel-danilewicz-consult-red Jul 31, 2024
dab52d2
Align line breaking algorithm for wrapWord
marcel-danilewicz-consult-red Aug 1, 2024
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
21 changes: 18 additions & 3 deletions examples/tests/text-contain.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ export default async function test(settings: ExampleSettings) {
fontFamily: 'Ubuntu',
textRendererOverride: 'sdf',
fontSize: 20,
text: `Lorem ipsum dolor sit e
text: `LoremipsumdolorsiteConsecteturadipiscingelit.Vivamusid.
Lorem ipsum dolor sit e
Consectetur adipiscing elit. Vivamus id.
Suspendisse sollicitudin posuere felis.
Vivamus consectetur ex magna, non mollis.`,
Expand Down Expand Up @@ -132,6 +133,7 @@ Vivamus consectetur ex magna, non mollis.`,
// SDF, contain none
text1.textRendererOverride = 'sdf';
text1.contain = 'none';
text1.wrapWord = 'normal';
text1.width = 0;
text1.height = 0;
},
Expand All @@ -153,11 +155,17 @@ Vivamus consectetur ex magna, non mollis.`,
// SDF, contain both (1 pixel larger to show another line)
text1.height = 204;
},
() => {
// SDF, contain both (1 pixel larger to show another line), wrap word
text1.contain = 'width';
text1.wrapWord = 'break';
},

() => {
// Canvas, contain none
text1.textRendererOverride = 'canvas';
text1.contain = 'none';
text1.width = 0;
(text1.wrapWord = 'normal'), (text1.width = 0);
text1.height = 0;
},
() => {
Expand All @@ -180,6 +188,11 @@ Vivamus consectetur ex magna, non mollis.`,
// Canvas, contain both (1 pixel larger to show another line)
text1.height = 204;
},
() => {
// Canvas, contain both (1 pixel larger to show another line), wrap word
text1.contain = 'width';
text1.wrapWord = 'break';
},
];
/**
* Run the next mutation in the list
Expand All @@ -202,6 +215,7 @@ Vivamus consectetur ex magna, non mollis.`,
text1.contain,
text1.width,
text1.height,
text1.wrapWord,
);
indexInfo.text = (i + 1).toString();
textSetDimsInfo.text = `Set size: ${Math.round(text1.width)}x${Math.round(
Expand Down Expand Up @@ -237,6 +251,7 @@ function makeHeader(
contain: string,
width: number,
height: number,
wrapWord: string,
) {
return `${renderer}, contain = ${contain}`;
return `${renderer}, contain = ${contain}, wrapWord = ${wrapWord}`;
}
11 changes: 11 additions & 0 deletions src/core/CoreTextNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export class CoreTextNode extends CoreNode implements CoreTextNodeProps {
textBaseline: props.textBaseline,
verticalAlign: props.verticalAlign,
overflowSuffix: props.overflowSuffix,
wrapWord: props.wrapWord,
});
this.textRenderer = resolvedTextRenderer;
this.trState = textRendererState;
Expand Down Expand Up @@ -346,6 +347,16 @@ export class CoreTextNode extends CoreNode implements CoreTextNodeProps {
}
}

get wrapWord(): CoreTextNodeProps['wrapWord'] {
return this.trState.props.wrapWord;
}

set wrapWord(value: CoreTextNodeProps['wrapWord']) {
if (this.textRenderer.set.wrapWord) {
this.textRenderer.set.wrapWord(this.trState, value);
}
}

get debug(): CoreTextNodeProps['debug'] {
return this.trState.props.debug;
}
Expand Down
1 change: 1 addition & 0 deletions src/core/Stage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,7 @@ export class Stage {
overflowSuffix: props.overflowSuffix ?? '...',
debug: props.debug ?? {},
shaderProps: null,
wrapWord: props.wrapWord ?? 'normal',
};

return new CoreTextNode(this, resolvedProps);
Expand Down
1 change: 1 addition & 0 deletions src/core/text-rendering/renderers/CanvasTextRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,7 @@ export class CanvasTextRenderer extends TextRenderer<CanvasTextRendererState> {
].join(' '),
textColor: getNormalizedRgbaComponents(state.props.color),
offsetY: state.props.offsetY,
wordBreak: state.props.wrapWord == 'break',
wordWrap: state.props.contain !== 'none',
wordWrapWidth:
state.props.contain === 'none' ? undefined : state.props.width,
Expand Down
16 changes: 14 additions & 2 deletions src/core/text-rendering/renderers/LightningTextTextureRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export class LightningTextTextureRenderer {
wordWrapWidth,
letterSpacing,
textIndent,
this._settings.wordBreak,
);
} else {
linesInfo = { l: this._settings.text.split(/(?:\r\n|\r|\n)/), n: [] };
Expand All @@ -347,6 +348,7 @@ export class LightningTextTextureRenderer {
wordWrapWidth - w,
letterSpacing,
textIndent,
this._settings.wordBreak,
);
usedLines[usedLines.length - 1] = `${al.l[0]!}${
this._settings.overflowSuffix
Expand Down Expand Up @@ -681,6 +683,7 @@ export class LightningTextTextureRenderer {
wordWrapWidth: number,
letterSpacing: number,
indent = 0,
wordBreak: boolean,
) {
// Greedy wrapping algorithm that will wrap words as the line grows longer.
// than its horizontal bounds.
Expand All @@ -691,11 +694,21 @@ export class LightningTextTextureRenderer {
const resultLines = [];
let result = '';
let spaceLeft = wordWrapWidth - indent;
const words = lines[i]!.split(' ');
const words = wordBreak ? [lines[i]!] : lines[i]!.split(' ');
Copy link
Contributor

@elsassph elsassph Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok that's kind of right and wrong - very interesting.

The good:

  • If we're breaking text we don't really care about spaces anymore; we can "just" find how many characters fit, and fill the lines,

The bad:

  • This is very inefficient,
  • It awkwardly integrates with the existing loop.

Suggestion:

if (wordBreak) {
  // approach wrapping the lines
} else {
  // original code splitting and wrapping the words
}

Performant approach:

  • This class has a wrapWord(word, wordWrapWidth, suffix) method which has an optimized algorithm to cut a string at a max width. It will return the part that fits, from which you can figure the remainder and repeat until everything has fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I tried the wrapWord, but the line break points are a bit different between sdf and canvas.
image
image
I'll push the commit if this is ok.

Copy link
Contributor

@elsassph elsassph Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm ok curious. Why is sit on the next line for SDF? And also why did it keep the space character when wrapping ex magna? Generally you want the wrapping logic to avoid wrapping one space to the next line.
Aside from the the canvas wrapping looks alright, if the definition is to fit as many characters per line as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got stuck a little bit on this issue. To prevent sit from being carried to the next line additional wrapWord=='normal' condition is needed in line 213:

 // Word wrap check
        if (
          // We are containing the text
          contain !== 'none' &&
          wrapWord == 'normal' &&

Then to break at the correct point
charEndX + glyph.width >= lineVertexW (line 299)
should be changed to
charEndX + glyph.width + glyph.xOffset + 4 >= lineVertexW

The value of 4 was concluded experimentally and I'm not even sure why it is needed there. Here you can see the how breaking looks without it:
image
and with:
image

Finally - I have no idea about additional space before "ex".

Copy link
Contributor

Choose a reason for hiding this comment

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

About sit, I guess we should either wrap or break works, no?

Adding +4 isn't worth it - it's just the subtle differences in how text layout happens.

For the space before ex it's probably because when you cut the line, it cuts just at the space characters, so line starts with a space. We shouldn't carry over a space character to next line when we break, so probably we can remove a starting space character at the beginning of a wrapped line.

for (let j = 0; j < words.length; j++) {
const wordWidth = this.measureText(words[j]!, letterSpacing);
const wordWidthWithSpace =
wordWidth + this.measureText(' ', letterSpacing);
if (wordBreak && wordWidthWithSpace > wordWrapWidth) {
let remainder = '';
while (this.measureText(words[j]!) > wordWrapWidth) {
remainder = words[j]!.slice(-1) + remainder;
words[j] = words[j]!.slice(0, -1);
}
if (remainder.length > 0) {
words.splice(j + 1, 0, remainder);
}
}
if (j === 0 || wordWidthWithSpace > spaceLeft) {
// Skip printing the newline if it's the first word of the line that is.
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 is where you cut the word to fix within the spaceLeft, and then what remains is cut if remainder still goes over wordWrapWidth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but what remains is cut in next iteration of for (let j = 0; j < words.length; j++) loop as it is inserted to words at j+1 index and thus treated as another word. I don't see any issue here, but maybe I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok it can work here

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I know that this project tends to use mostly visual regression tests, but this is a case where some unit tests would be greatly beneficial @wouterlucas

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW I know that this project tends to use mostly visual regression tests, but this is a case where some unit tests would be greatly beneficial @wouterlucas

theres a vitest infrastructure as well, just add *.test.ts and it should be picked up

// greater than the word wrap width.
Expand All @@ -720,7 +733,6 @@ export class LightningTextTextureRenderer {
realNewlines.push(allLines.length);
}
}

return { l: allLines, n: realNewlines };
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/text-rendering/renderers/SdfTextRenderer/SdfTextRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,7 @@ export class SdfTextRenderer extends TextRenderer<SdfTextRendererState> {
scrollable,
overflowSuffix,
maxLines,
wrapWord,
} = state.props;

// scrollY only has an effect when contain === 'both' and scrollable === true
Expand Down Expand Up @@ -569,6 +570,7 @@ export class SdfTextRenderer extends TextRenderer<SdfTextRendererState> {
scrollable,
overflowSuffix,
maxLines,
wrapWord,
);

state.bufferUploaded = false;
Expand Down
19 changes: 19 additions & 0 deletions src/core/text-rendering/renderers/SdfTextRenderer/internal/layoutText.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export function layoutText(
scrollable: TrProps['scrollable'],
overflowSuffix: TrProps['overflowSuffix'],
maxLines: TrProps['maxLines'],
wrapWord: TrProps['wrapWord'],
): {
bufferNumFloats: number;
bufferNumQuads: number;
Expand Down Expand Up @@ -292,6 +293,24 @@ export function layoutText(
maxX = Math.max(maxX, quadX + glyph.width);
curX += glyph.xAdvance;
}
if (
marcel-danilewicz-consult-red marked this conversation as resolved.
Show resolved Hide resolved
wrapWord == 'break' &&
contain != 'none' &&
charEndX + glyph.width >= lineVertexW
) {
if (curLineBufferStart !== -1 && lineIsWithinWindow) {
bufferLineInfos.push({
bufferStart: curLineBufferStart,
bufferEnd: bufferOffset,
});
curLineBufferStart = -1;
}
curX = 0;
curY += vertexLineHeight;
curLineIndex++;
lastWord.codepointIndex = -1;
xStartLastWordBoundary = 0;
}
} else {
// Unmapped character

Expand Down
9 changes: 9 additions & 0 deletions src/core/text-rendering/renderers/TextRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,12 @@ export interface TrProps extends TrFontProps {
* @default 'none'
*/
contain: 'none' | 'width' | 'both';
/**
* Word wrap option
*
* @default normal
*/
wrapWord: 'normal' | 'break';
width: number;
height: number;
/**
Expand Down Expand Up @@ -370,6 +376,9 @@ const trPropSetterDefaults: TrPropSetters = {
contain: (state, value) => {
state.props.contain = value;
},
wrapWord: (state, value) => {
state.props.wrapWord = value;
},
offsetY: (state, value) => {
state.props.offsetY = value;
},
Expand Down
Loading