Kerning implementation open for discussions

[swingingtom]

Hey, as the PR titled it, the current implementation of kerning raised some questions from my side, and therefore require others opinions and decisions.

The current implementation is functional, all examples are still up ( this time... ). Some parts are not production ready (ie: overlap toggler).

Questions

Attribute name

Lets start by an easy one: kerning, fontKerning , somethingElse ?
As inline elements doesn't always means a Text I slightly prefer fontKerning

Default value / Optional

What should the default value ON or OFF ?
Kerning are made to offer better visual looking for particular character combinaisons. So I would think it would be better to set the kerning ON by default. But in the other hand this might slighty affect users who update the library on a existing project.

And if ON by default, should we still be able to optionally disable kerning ? Its always great to have options, but assuming it always on, it could ease the codebase. How many users will disable kerning (which improves visual looking)? In what purpose?

Friendly font kerning values lookup ( ok? going further? )

MSDF json stores kernings values in array

In order to ease selecting kerning values, the current implementation adds an internal object

Any kerning value of zero, is ignored. The number of kerning items drops from 3674 to 1856 (on the roboto font included, which contains a huge charset)

And the lookup object is eased IMO

const glyphPair = "ij";

// unaltered msdf kerning array
const kerningAmount = font.kernings.find( k =>  k.first == glyphPair.charCodeAt(0) && k.second == glyphPair.charCodeAt(1)  ).amount;

//friendly kerning object
const kerningAmount = font._kernings[glyphPair] ? font._kernings[glyphPair] : 0; 

So, what do you think about that ?
Would it go further ? Removing the original msdf kerning array from memory to store altered kerning object instead ? Having an internal Font class ?

Kerning location

Retrieving kerning has been implemented outside of const glyphInfos = chars.map( (glyph)=> {...}).

const chars = Array.from ? Array.from( content ) : String( content ).split( '' );
const glyphInfos = chars.map( (glyph)=> { /*[...]*/ });

// then apply kerning
if( this.getFontKerning() ){

	// First character won't be kerned with its void lefthanded peer
	for (let i = 1; i < glyphInfos.length; i++) {

		const glyphInfo = glyphInfos[i];
		const glyphPair = glyphInfos[i-1].glyph+glyphInfos[i].glyph;

		const kerning = this.getGlyphPairKerning( textType, font, glyphPair);
		glyphInfo['kerning'] = kerning * fontSize * glyphInfo['width'];

	}
}

Is font kerning option on is only checked once, and no checks to avoid kerning the first character.

While retrieving kerning inside would have required :

const glyphInfos = chars.map( (glyph, index)=> {
    
    let glyphPair = glyph;
    if( index > 0 )
    {
        glyphPair = glyphInfos[index-1]+glyph;
    }

    //[...]
}

It annoyed me a bit that any glyph in a paragraph would have to check index>0 when even a huge text, will only need to do the exception only for the first character. (lefthanded peer doesn't exists). Same apply for checking if kerning attribute is ON.

Overlapping glitches

As the kerning is often negative values, some characters 'quad?' might overlap its previous character 'quad?'.

I currently fixed it by setting the fontMaterial.depthWrite = false which may have side effects for several users.

I dont remember exactly what you said, but I remember to read something about dat.gui/lil.gui in an issue, so instead of adding a gui, I added a command on the window element. In the example 'font_kerning' we can type overlap in the console, and it will toggle on/off fontMaterial.depthWrite property on Text Components. Be sure the be on the font_kerning frame in the console.

Notes

FontLibrary

For friendly kerning lookup, I implemented it on FontLibrary. It confuse me a bit at first, because even if FontLibrary is a mandatory gateway, it has multiple available gates (addFont(),loadFontJson(),setFamilyFont()) without a centralized point of registering a fontJson.

Having an abstract FontClass could be the centralized point. Then instead of asking TextManager.getGlyphDimension() which check textType to go on MSDFText.getGlyphDimension() it could rely on abstract method of FontClass.getGlyphDimension() and MSDFFont extends FontClass. Let's be clear: It's core changes, and clearly not the point in this PR. Just the feeling it miss something to ease future implementations.

Linebreak can be the first character of a line

Not totally related to kerning, but sometimes I have the first character of a line that is a '\n'. LetterSpacing was introducing a bug when this case appears. A fix is made in this PR, as kerning also have to be taken into account.

if (
		lastInlineOffset + inline.width > INNER_WIDTH ||
		inline.lineBreak === "mandatory" ||
		this.shouldFriendlyBreak( inlines[ i - 1 ], lastInlineOffset, nextBreak, INNER_WIDTH )
	) {

		lines.push([ inline ]);

		inline.offsetX = 0;

		// @TODO: How to prevent this
               // "\n" glyph has a width of zero and appears to be first glyph of a newline
		if( inline.width > 0 ){

			// Do not kern first letter of a line, its has not lefthanded peer
			return inline.width + letterSpacing;
		}else{
			// When line breaker here "\n" its width is 0
			// but letterSpacing was still adding constant offset on empty char (Fixed)
			return 0;
		}
	} 

The check if( inline.width > 0 ) is only run when once per line, so it's not a performance issue, but the current resolution is not intuitive. Even being commented.

Indentation

Some files are indented with tabs, while other with spaces. Honestly I don't care, just noticing.

TLDR; kerning was indeed an easy task, but I won't decide the path it should follow by my own.

[swingingtom]

#17

[swingingtom]

Poor's man gui

[felixmariotto]

Hi !

First of all let me say that it looks really better with kerning, thanks a lot for digging into this.

So to answer your points :

Attribute name

As for the attribute name I vote for fontKerning since it's similar to the CSS font-kerning and therefore makes it intuitive for new users.

Default value / Optional / Overlapping glitches

The overlapping glitch is a big issue in my opinion... and setting fontMaterial.depthWrite = false by default will indeed cause havoc. Because of that, I think kerning should be optional and default to "off", and fontMaterial.depthWrite be false only when kerning is on.

Also if not wanted the user could trim out the kernings array from their font file, which makes it much smaller. At the moment it's a manual process, but as you mentioned in #90 we could have our own generator which could make kerning an option.

As for fontKerning value, what do you think about copying the CSS values "normal" and "none" ? This way we can add a value in the future like "auto" without breaking old code.

Friendly font kerning values lookup

The lookup object is a wonderful idea IMO ! In cases like text editors or timers, updates to the Text might come very often, so optimizing this is a good idea indeed. As for removing the original kernings array I don't know, I don't really have an opinion on this. I guess we could remove it to save some memory ? What is your own feeling about this ?

Font class

I can understand why you where confused about where to put your code in FontLibrary... I also think that a Font class would ease future work, notably in adding font types. Let's handle this in a separate PR though.

Suggestions

The workaround in InlineManager is acceptable but could you improve a bit the comments to help future maintenance please ?
Also don't forget to add a default to fontKerning in the Default module.
About the example's text, while your text nicely demonstrates kerning's power for readability, it is not very educational. Something like "fontKerning adjusts the space between letters to make them fit perfectly", with and whithout kerning, would in my opinion be more useful for beginners.

[swingingtom]

The overlapping glitch is a big issue in my opinion... and setting fontMaterial.depthWrite = false by default will indeed cause havoc. Because of that, I think kerning should be optional and default to "off", and fontMaterial.depthWrite be false only when kerning is on.

Let's go with your suggestion for now, and if you agreed, we could ask on three.discourse what our options are. I could do a codesandbox to illustrate the issue with some gui, and if we get lucky, we could have a threejs maintainer giving options/advices.

As for removing the original kernings array I don't know, I don't really have an opinion on this. I guess we could remove it to save some memory ? What is your own feeling about this ?

Internally, ThreeMeshUI could only keep data that is currently used. Users could still have the full source json stored in their "user application level". Let's keep more time to think about it, and try to plan it when/if we do something like FontClass.

About the example's text, while your text nicely demonstrates kerning's power for readability, it is not very educational. Something like "fontKerning adjusts the space between letters to make them fit perfectly", with and whithout kerning, would in my opinion be more useful for beginners.

Unfortunately, despite the text being useful, the characters combinaisons behind, rendered with the provided font do not show any kerning at all.

[felixmariotto]

Hey no worries for the occlusion, I thought that it would cause issues with other meshes, but it doesn't :

So finally you can make "kerning on" the default. Please keep it optional though, because as I said removing kerning data from the font JSON makes it much lighter and it might be important for some users using large charsets (chinese characters, etc..)

As for the text, let's keep your original suggestion for now.

[swingingtom]

But this still have side effects. fontMaterial.depthWrite = false will prevent the renderer to write depth informations about each character. ( Im not threejs nor 3d expert, so correcting me here is more than welcome )

Such library as https://github.com/vanruesc/postprocessing or threejs postprocessing effects requires depth informations to compute the effect (GodRays, Bloom, etc... )

At first I workaround the overlap using fontMaterial.depthTest = false. It solved it, but Text was never occluded by a geometry in front of it. Except after setting renderOrder of each elements (Which was pretty unintuive : onAfterUpdate, check that Text as Mesh children to apply desired renderOrder).

Maybe something like discarding rendering of transparent pixels in the shader will have the best flexibility. But thats a guess, Im not sure it will.

[felixmariotto]

From my testing fontMaterial.depthWrite = false seems to still allow occlusion of the text mesh by meshes in front, and the text still occlude meshes at the back. It just removes self-occlusion.

will prevent the renderer to write depth informations about each character.

Not sure we want that anyway, the characters quads are merged into a single mesh (single draw call, better perf).

Maybe something like discarding rendering of transparent pixels in the shader will have the best flexibility. But thats a guess, Im not sure it will.

Maybe yes, it's worth a try.

[felixmariotto]

It seems that fontMaterial.depthWrite = true and adding if ( alpha < 0.02 ) discard; here does exactly what we want :

[swingingtom]

The suggestions discussed here have been implemented.
Sould I add new issues such as :

• Implementing fontKerning auto
• Prevent a line to start with "\n"
• Think about a better text to illustrate fontKerning example
• ...

[felixmariotto]

Hi @swingingtom, it seems all good to me, but I just noticed a small issue I don't understand.

In the InlineBlock example, there is one bit of text that should be kerned:

If I look for the word "emojis" with the Roboto font on Google Fonts, the "o" and "j" are a kerned pair:

I don't know if it's just an oversight in a previous version of the font, the evidence of a problem on our side, or maybe an issue with the generator. I checked and it looks like there is no "oj" pair in the font object after parsing by your _buildFriendlyKerningValues function.

[felixmariotto]

Also it seems that kerning does not take the font size into account :

[swingingtom]

"oj" situation
I've analyzed the complete kerning information prodived by the ./assets/Roboto-msdf.json font. It doesn't contains "oj" pair. So it's not something truncated during _buildFriendlyKernings() computation.

On google font, I've toggled font-kerning css property. Those nice "oj" we can see are not coming from kernings.

I would have guess it could comes from ligatures.
But even by disabling ligatures on google fonts, it still display a nice "oj" pair.

From what I can see, google font roboto, and provided roboto are not the same. "i" or "j" dots appears square on ThreeMeshUi examples, and completely round from google font.

Do you remember where did you get the roboto source before msdf conversion ?

fontSize issue
As it currently took fontSize property into account, I could only guess that the current kerning computation is wrong. I will dive a bit deeper into this

[felixmariotto]

Do you remember where did you get the roboto source before msdf conversion ?

I think it was Google Fonts.

I tried to download Roboto from Google Fonts and to generate the msdf again, but the issue persists:

[felixmariotto]

Same issue with another font (Lato) :

[swingingtom]

Im not sure this related to current kerning implementation, we can already notice this disgraceful space from current release https://felixmariotto.github.io/three-mesh-ui/#inline_block

For further implementations, It would be great to have an html text reference overlay that display the same texts. We would then try to fit it as much as possible.

Im currently struggling to have some documentation on the msdf format itself.

Wouldn't be possible that this disgraceful space is caude by the fact that ThreeMeshUi use char.width instead of char.xadvance in order to compute nextChar.x ?

Also, do you know if common.base represent the em reference value? Those values match, but I don't want to assume it. If you have some msdf format guide explained, or something detailing the format, it will still be christmas...

[felixmariotto]

Wouldn't be possible that this disgraceful space is caude by the fact that ThreeMeshUi use char.width instead of char.xadvance in order to compute nextChar.x ?

Yes it's certainly the issue. This is indeed unrelated to the present PR so let's not worry about this now, I opened a new issue: #108

As for what msdf-bmfont-xml outputs, I must admit I'm not sure what standard it follows... I found this doc which was helpful but doesn't seem to be a very reliable source.

[swingingtom]

Thanks !
The angelcode document helps a lot !

[swingingtom]

Hey ! First and to be honest, Im not sure where this PR is going to. It may be better, to abort it. Then redo-ing one issue at the time.

I succeed to have some good results, but it leads slightly out of the scope.

What I really needed was to have an html overlay showing the same text with same properties as ThreeMeshUI but in html.
I then had something to visually control what it should looks like. For each texts. Everytimes. For now, its just a quick and dirty code, but I think a real useable piece for both developers and users would be an asset. Filled with gui controls, and other options, just like an editor, studio, ...

Bref, current setup ( font_kerning example file ) :

• orthographic camera to reduce perspective deformation
• Exported roboto 500 (medium) to be sure same font with html. I added [space] char in the msdf charset to rely on the space char defined in the font.
• Font displayed in 42px ( original size of the msdf export ) - Scale Factor is then 1, minimizing approximation errors ( accumulations )

current results :

• Red html, white ThreeMeshUI
• High fidelity with and without kerning. ( kerning normal above, kerning none below )
• Still shows accumulations erros. It highly depends on the text itself.

Sequence of "i" or "j" tends to add a lot of approximation errors :

Changes

Scale Factor

Rely on msdf.info.size

const SCALE_MULT = FONT_SIZE / FONT.info.size;
//[...]
let width = charOBJ ? charOBJ.width * SCALE_MULT : FONT_SIZE / 3 ;
// instead of
let width = charOBJ ? (charOBJ.width * FONT_SIZE) / FONT.common.lineHeight : FONT_SIZE / 3 ;

xadvance and xoffset

Chars position x relies on xadvance and xoffset ( + kerning & + letterspacing ).

inline.offsetX = lastInlineOffset + xoffset + kerning;
// and listInlineOffset become                
return lastInlineOffset + xadvance + kerning + letterSpacing;

What could be an issue (performance and maintenance), is that inlineManager do computation for other kind of component than Text. I understand it now.
Text and InlineBlock components go through same methods (ie: inlineComponent.inlines.reduce ) and therefore assuming inline is a glyph could leads to runtime errors (NaN).
So checks of values are required here, here and here

Other examples files

Globally, it impacts every file. Even with kerning set to "normal", texts are more spaced than it was before. Leading to example having more lines than before.
New code old font (computed space size) :

Prod code old font :

Which goes fine again with the fresh export of Robot Medium font with space char included. Even with font being boldier

Also note that xadvance and xoffset solved the "emoji" disgrace.

Lineheight

I didn't change anything (at least I don't think so), but it currently doesnt match html. It's especially strange that lineheight can currently differs depending on which chars are on that line. Lines full of uppercase or lowercase, and also the previous screenshot also display it :

Issue

On the nested block example, we can notice the behaviour commented here. But this time with a space char.

Approximation

As said before, there is still rendering differences between current implementation and html. Especially after many characters, we see it growing.

Just a random guess, but as msdf converter tends to works in integers ( width ok, but xadvance, xoffset, kerning amount) we could try to set the initial size. Currently its automatically set to 42. Which may be not the perfect number. I think about 48 which is a lot more divisible than 42.

That's it.
Thank you for the https://www.angelcode.com/products/bmfont/doc/file_format.html link ! It really helped

[felixmariotto]

OK, I just merged after fixing the examples where the new spacing introduced issues, because you're right it was becoming out of hand !
I pushed as 6.0.0 since this is a breaking update (old layouts will break with the same code, just like the examples).

I changed the kerning example to make it only show the feature, but your example with an HTML overlay over three-mesh-ui is very interesting to test the difference between the 2. If you want to make a new PR to add a new "html comparison sandbox" example, maybe with an HTML input field so users can test any text, that would be very useful IMO.

Thanks a lot for all this work, the readability of the text has improved a lot !