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 6 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
14 changes: 13 additions & 1 deletion 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 @@ -696,6 +699,16 @@ export class LightningTextTextureRenderer {
const wordWidth = this.measureText(words[j]!, letterSpacing);
const wordWidthWithSpace =
wordWidth + this.measureText(' ', letterSpacing);
if (wordWidthWithSpace > wordWrapWidth && wordBreak) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put conditions in narrowing order:

if (wordBreak && the rest) {

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 the algorithm here is different from SDF.

Isn't the goal agreed to cut the word so the beginning of the word fits at the end of the line? e.g. keep what fits within spaceLeft and move the remainder to the next line (where indeed it could still be > 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' &&
charEndX + glyph.width >= lineVertexW &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that correct? Wrap-word I think is meant to only wrap a word when this word alone is larger than the wrapping width.

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 thought it should be wrapped if the width is exceeded. So if a word doesn't fit it should be moved to the next line and then if still doesn't fit it should be wrapped?

Copy link
Contributor

@elsassph elsassph Jul 18, 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 it would be silly to break every word. The point is that sometimes (as reported in #268) you may have a single word larger than the target width, in which case you may prefer to break it.

In L2, that's what wordBreak option does. I recommend checking the implementation (which is TBH rather inefficient). The algorithm actually breaks words "before" doing the layout, so for instance a very long word would be always cut to not exceed the full width, but it won't try to fit a few characters on the previous line. It also supports words being multiple times longer than the available width - imagine for instance using this feature to render text vertically, one letter per line.
See: https://github.com/rdkcentral/Lightning/blob/dev/src/textures/TextTextureRendererAdvanced.mjs#L175

I suggest again to make sure that the requirements are non-ambiguous - ask @wouterlucas for confirmation. Make a diagram/drawing to explain what you are going to do.

In any case it is very wise to have proposed an early draft.

contain != 'none'
) {
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