Implement wrapWord parameter (#268) #345

marcel-danilewicz-consult-red · 2024-07-17T11:35:11Z

WiP, todo: canvas renderer

elsassph · 2024-07-17T13:36:12Z

src/core/text-rendering/renderers/SdfTextRenderer/internal/layoutText.ts

@@ -292,6 +293,24 @@ export function layoutText(
          maxX = Math.max(maxX, quadX + glyph.width);
          curX += glyph.xAdvance;
        }
+        if (
+          wrapWord &&
+          charEndX + glyph.width >= lineVertexW &&


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

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?

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.

elsassph · 2024-07-29T13:28:57Z

src/core/text-rendering/renderers/LightningTextTextureRenderer.ts

@@ -720,6 +723,18 @@ export class LightningTextTextureRenderer {
        realNewlines.push(allLines.length);
      }
    }
+    if (wordBreak) {


I think this is incorrect in some cases, and inefficient in general. You should instead break words as part of the wrapText function.

But this is inside of wrapText function :)
Could you give me an example of NG case for this code?

Right, but it is misplaced I think: it re-measures every word which we already measure in the above loop - could the break happen as part of the above wrapping logic? Also isn't that a different breaking logic as SDF? It seems SDF tries to cut the word to fit the line, isn't it?

It remeasures whole lines. I'd rather separate normal and break cases, but you are right that the code can be optimized. Algorithms are not the same due to different surrounding code.

No allLines has all the individual words, and realNewlines tells the index of each new line's first word.

I changed the algorithm in last commit. It should be better now, but words still need to be measured in order to find the breaking point. Probably it can be optimized further , but I don't see a way to entirely avoid measurement.

elsassph · 2024-07-30T11:17:20Z

src/core/text-rendering/renderers/LightningTextTextureRenderer.ts

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


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.

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.

Ok it can work here

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

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

src/core/text-rendering/renderers/SdfTextRenderer/internal/layoutText.ts

elsassph · 2024-07-31T08:17:57Z

src/core/text-rendering/renderers/LightningTextTextureRenderer.ts

@@ -696,6 +699,16 @@ export class LightningTextTextureRenderer {
        const wordWidth = this.measureText(words[j]!, letterSpacing);
        const wordWidthWithSpace =
          wordWidth + this.measureText(' ', letterSpacing);
+        if (wordWidthWithSpace > wordWrapWidth && wordBreak) {


Put conditions in narrowing order:

if (wordBreak && the rest) {

elsassph · 2024-07-31T08:20:55Z

src/core/text-rendering/renderers/LightningTextTextureRenderer.ts

@@ -696,6 +699,16 @@ export class LightningTextTextureRenderer {
        const wordWidth = this.measureText(words[j]!, letterSpacing);
        const wordWidthWithSpace =
          wordWidth + this.measureText(' ', letterSpacing);
+        if (wordWidthWithSpace > wordWrapWidth && wordBreak) {


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)

elsassph · 2024-08-01T13:18:09Z

src/core/text-rendering/renderers/LightningTextTextureRenderer.ts

@@ -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(' ');


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.

Good point. I tried the wrapWord, but the line break points are a bit different between sdf and canvas.

I'll push the commit if this is ok.

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.

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:

and with:

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

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.

pecoram · 2024-11-05T11:22:27Z

What is missing to complete this PR? @marcel-danilewicz-consult-red

marcel-danilewicz-consult-red added 2 commits July 12, 2024 13:33

Add wrapWord option, no implementation yet

da89c2e

Implement wrapWord for sdf text renderer

98b45c9

elsassph reviewed Jul 17, 2024

View reviewed changes

wouterlucas mentioned this pull request Jul 25, 2024

Wrong wrapping with contain width and text too long #268

Open

marcel-danilewicz-consult-red added 3 commits July 26, 2024 13:59

Change wrapWord to string literal

8a939ad

Implement wrapWord for canvas text renderer

4f47680

Add wrapWord parameter to text-contain example

c7f8c9c

elsassph reviewed Jul 29, 2024

View reviewed changes

Improve breaking algorithm

856cc7c

elsassph reviewed Jul 30, 2024

View reviewed changes

elsassph reviewed Jul 31, 2024

View reviewed changes

src/core/text-rendering/renderers/SdfTextRenderer/internal/layoutText.ts Show resolved Hide resolved

elsassph reviewed Jul 31, 2024

View reviewed changes

marcel-danilewicz-consult-red added 2 commits August 1, 2024 13:47

Change order of conditions

3c98b21

Align line breaking algorithm for wrapWord

dab52d2

elsassph reviewed Aug 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement wrapWord parameter (#268) #345

Implement wrapWord parameter (#268) #345

marcel-danilewicz-consult-red commented Jul 17, 2024

elsassph Jul 17, 2024

marcel-danilewicz-consult-red Jul 18, 2024

elsassph Jul 18, 2024 •

edited

Loading

elsassph Jul 29, 2024

marcel-danilewicz-consult-red Jul 29, 2024

elsassph Jul 29, 2024 •

edited

Loading

marcel-danilewicz-consult-red Jul 30, 2024 •

edited

Loading

elsassph Jul 30, 2024

marcel-danilewicz-consult-red Jul 30, 2024

elsassph Jul 30, 2024

marcel-danilewicz-consult-red Jul 30, 2024 •

edited

Loading

elsassph Jul 31, 2024

elsassph Jul 31, 2024

wouterlucas Sep 2, 2024

elsassph Jul 31, 2024

elsassph Jul 31, 2024

elsassph Aug 1, 2024 •

edited

Loading

marcel-danilewicz-consult-red Aug 2, 2024

elsassph Aug 2, 2024 •

edited

Loading

marcel-danilewicz-consult-red Aug 7, 2024

elsassph Aug 7, 2024

pecoram commented Nov 5, 2024 •

edited

Loading

Implement wrapWord parameter (#268) #345

Are you sure you want to change the base?

Implement wrapWord parameter (#268) #345

Conversation

marcel-danilewicz-consult-red commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsassph Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsassph Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

marcel-danilewicz-consult-red Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcel-danilewicz-consult-red Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsassph Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsassph Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pecoram commented Nov 5, 2024 • edited Loading

elsassph Jul 18, 2024 •

edited

Loading

elsassph Jul 29, 2024 •

edited

Loading

marcel-danilewicz-consult-red Jul 30, 2024 •

edited

Loading

marcel-danilewicz-consult-red Jul 30, 2024 •

edited

Loading

elsassph Aug 1, 2024 •

edited

Loading

elsassph Aug 2, 2024 •

edited

Loading

pecoram commented Nov 5, 2024 •

edited

Loading