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

Feature: MSDF support on BitmapText #7781

Merged
merged 12 commits into from
Sep 20, 2021

Conversation

miltoncandelero
Copy link
Contributor

@miltoncandelero miltoncandelero commented Sep 7, 2021

Description of change

This is my initial work and POC for adding MSDF on top of all the features BitmapText already brings to the table.

More information on SDF and MSDF can be found on the following links:

This PR is a draft since still have some work to do and I want to get some input on some decisions.


Questions I have:
  • Using only one shader for MSDF and SDF cause visual degradation on non MTSDF assets, Should we have 3 similar but different shaders (MSDF, SDF and MTSDF) or should we have only one shader that can pick the smoothing needed with some step trickery?

  • BitmapText and MSDFText are only different in the shader they use to render. For now, MSDFText extends from BitmapText and rewrites updateText to be almost exactly the same but using a different shader. Should MSDF be an internal switch to flip inside a BitmapText when we detect that a BitmapFont has the fields for Distance Field rendering?

    • Suggestion 1: Keeping a Dictionary of Pools for a mesh object and changing the shader depending on the kind of BitmapFont we are rendering
    • Suggestion 2: Exposing the program that the BitmapText will use for its shader allowing for easier extension without copy-pasting and entire method
  • Is it a bad practice to change a float uniform every frame? Right now during render a new value is fed to the uniform without checking if it was the same. I don't think this is a problem but I am not entirely sure.


Proof of concept:

Atlas were created using msdf-bmfont-xml with a fontsize of 64 px.
(The demo works regardless of pixel density)
msdf test

(Please note: SDF looks wrong because of the single shader trying to apply MSDF on an SDF image. See the questions section above for more info.)

direct HTML demo, just host the contents. The demo includes a build of pixi.js with the latest changes.
msdfTest.zip


Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@bigtimebuddy
Copy link
Member

I think we are due for an official signed distance field text rendering approach. So thanks for taking a stab at this. Tethering it to BitmapText is a clever approach so you don't need to reinvent the layout code. Do bitmap font tools (bmGlyph, etc) support distance field generation?

I do have a few pieces of general feedback.

BitmapText and Text are both canvas AND webgl supported. This is introducing a new text rendering approach that is webgl-only. This is going to create confusion and expectations of it working on canvas. My suggestion is another package that is opt-in (like events, graphics-extras or math-extras). In a the next major version that is webgl-only, we can make it the default behavior, but we are not there yet.

Eventually, having this be automatically detected (feature of BitmapText) seems better than having another explicit type. But switching between shaders (between SDF font and normal font) maybe a problem, no?

Also, the name is a tricky one. I find the MSDFText to be kind jargon-y for the average user and not that helpful in explaining why you'd want to use over BitmapText. Are there simpler, clearer options?

@miltoncandelero
Copy link
Contributor Author

@bigtimebuddy we can move to an approach similar to the smooth-graphics package.

Something like smooth-text-bitmap: a drop-in replacement for BitmapText?

@ivanpopelyshev
Copy link
Collaborator

we are adding new feature, not modifying current ones, we already have many features in pixi that are not supported in canvas mode.

If user gives SDF/MSDF font to pixi, he knows that it will look strange in canvas mode.

I dont see any reason to be defensive here. We don't need a drop-in, we just have to test this thing and publish it.

@bigtimebuddy
Copy link
Member

We know that MSDF/SDF text works in theory and in practice (heck, there are already plugins that do it). Our job is to find a way to thoughtful integrate this approach without confusing people. This is not a use-case for "throw it in, see what happens".

Also, the mental model for webgl vs canvas in pixi is pretty clear for high-level API objects like this: basically filters don't work in canvas, but all the other display objects should (Sprite, Mesh, Graphics, Container, Text, BitmapText). Sure, there are lots of low-level things, but introducing a new Text object that's webgl-only needs to be done thoughtfully. In my opinion this functionality should be opt-in for v6.

My most important concerns about this are naming (which I mentioned above), release strategy (how and when do we add it), and developer experience (no dramatically changing our developer contract that the high-level APIs work in canvas and webgl).

@miltoncandelero
Copy link
Contributor Author

miltoncandelero commented Sep 8, 2021

My most important concerns about this are naming (which I mentioned above), release strategy (how and when do we add it), and developer experience (no dramatically changing our developer contract that the high-level APIs work in canvas and webgl).

  • Naming: I agree. By overloading BitmapText and having it change internally if it is DF or not based on the font should make it so that you can use BitmapText without ever knowing Distance Fields existed and when you load a font that has a DF of any kind, magically works.
  • Release strategy: I think I agree with Ivan here. The inner functionality of BitmapFont should remain unchanged on canvas. Today if you load a DF font on canvas you will see it exactly as the image file shows it. After this change, while webgl would see the smooth font canvas will still see the image asset exactly as they did before this change.
  • Developer experience: If your bmfont supports any kind of distance field it will be stored inside the fnt file and the loader will save that into the BitmapFont object allowing for rendering a BitmapText without any kind of extra tweaking. It would be a "just works" scenario.

@ivanpopelyshev
Copy link
Collaborator

BitmapText works with canvas the same way Mesh does. If you specify special shader arguments in mesh - they wont work in canvas.

As for naming - zip file in first post is a bit outdated - if you look in PR - whole functionality is pair of extra IF's in BitmapText.

The only question to this implementation is that new Program(msdfVert, msdfFrag) will actually create a new program. We have to cache it somewhere, and ensure that sprite batching is enabled when SDF param is 0.

@miltoncandelero
Copy link
Contributor Author

The only question to this implementation is that new Program(msdfVert, msdfFrag) will actually create a new program. We have to cache it somewhere, and ensure that sprite batching is enabled when SDF param is 0.

I did this to not keep more than one mesh pool but now that you mention it, maybe two pools (one for sdf meshes and one for batched regular meshes) should be a cheap price to pay 🤔

@ivanpopelyshev
Copy link
Collaborator

Program has to be one. Its compiled (msdfVert,msdfFrag), independent from uniforms. It has to be done only once.

material should return "batchable=true" or how its done ( i dont exactly remember) if shader uniform uFWidth is 0. Then if text has 100 vertices (~25 symbols) it will be added to sprite batch, with our regular shader. Otherwise, it will be rendered with this shader, and honestly, for more than 25 symbols, shader switch and extra drawcall shouldnt be a problem.

@ivanpopelyshev
Copy link
Collaborator

Even more, when you do "new Program" its not actually compiled, its lazy, so you can do it anywhere, even in BitmapText.msdfProgram = ...

- Only one program is created
- BitmapTexts that don't need sdf treatment are batched with the default shader
  - Thanks to the `batchable` field of the MeshMaterial the sdf shader was simplified.
- NPM mode is correctly set for the meshes rendering SDF
  - Textures used in SDF are set to NPM inside the BitmapFont loader ONLY if detected to be SDF
@miltoncandelero
Copy link
Contributor Author

New working demo:
msdfTest2.zip
Improvements:

  • Only one program is created
  • BitmapTexts that don't need sdf treatment are batched with the default shader
    • Thanks to the batchable field of the MeshMaterial the sdf shader was simplified.
  • NPM mode is correctly set for the meshes rendering SDF
    • Textures used in SDF are set to NPM inside the BitmapFont loader ONLY if detected to be SDF

The demo includes a simple way to test all possible combinations between the current release and this SDF branch both in webGL and canvas. Screenshots attached for comparison

Release 6.1.2 SDF Branch
WebGL pixijs release pixijs sdf
Canvas pixijs legacy release pixijs legacy sdf

As we can see, this change only brings improvements to webgl while changing nothing to the canvas renderer (it looks bad in both versions)

On the topic of performance and batching, this is a screenshot of spector.js where we can see that the two normal bitmap texts and a simple sprite were batched together and then the custom shader rendered the two other texts (and finally the Text renderer).

Tall image that would break the flow of this post


I still need to write tests, torture test the loader/parser and cleanup my code but I think this addresses the main issues raised by @bigtimebuddy

Let me know if you have any issues! 😄

@bigtimebuddy bigtimebuddy added this to the v6.2.0 milestone Sep 8, 2021
@ivanpopelyshev
Copy link
Collaborator

Looks like current BitmapText implementation doesnt work properly in canvas too, its going through Mesh rendering instead of many small drawImage's. We broke it somewhen before

@miltoncandelero
Copy link
Contributor Author

  • Test added for loading Fonts.
  • Moved the responsibility of making the texture NPM from the loader to the BitmapFont constructor.
  • Added documentation on how to create an SDF bitmap font.

I think this is enough for the initial support.

In the future, we can try to create SDF fonts on the fly in the BitmapFont.from method by rendering big and smoothing from there (similar to what Hiero does).

Also in the future, we could write a custom batch renderer to render all SDF meshes, regardless of if they are from a BitmapText or not.

@miltoncandelero miltoncandelero marked this pull request as ready for review September 11, 2021 00:35
@GoodBoyDigital
Copy link
Member

this looks great @miltoncandelero ! going to find some time to review this weekend :D

@miltoncandelero
Copy link
Contributor Author

I had to remove the json support for MTSDF fonts from https://github.com/Chlumsky/msdf-atlas-gen

I got the fontdata to work but the exported json doesn't have any reference to the png output inside. Coding it so that your png has to be named like you json feels janky to me. If we are cool with this jankyness I can add the json parser back in.

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

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

this is really nice work @miltoncandelero ! we have been looking to get this in for ages :D
left a comment, main thing is that I feel we should still keep the default mesh shader around for regular text if going through the if (distanceFieldType === 'none') path.

@@ -65,6 +70,7 @@ const charRenderDataPool: CharRenderData[] = [];
*/
export class BitmapText extends Container
{
private static msdfProgram = new Program(msdfVert, msdfFrag);
Copy link
Member

Choose a reason for hiding this comment

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

no need to store this as we can leverage the Program Cache Program.from(msdfVert, msdfFrag)

@@ -400,7 +406,8 @@ export class BitmapText extends Container
if (!pageMeshData)
{
const geometry = new MeshGeometry();
const material = new MeshMaterial(Texture.EMPTY);
const material = new MeshMaterial(Texture.EMPTY,
{ program: BitmapText.msdfProgram, uniforms: { uFWidth: 0 } });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ program: BitmapText.msdfProgram, uniforms: { uFWidth: 0 } });
{ program: Program.from(msdfVert, msdfFrag), uniforms: { uFWidth: 0 } });


if (distanceFieldType === 'none')
{
for (const mesh of this._activePagesMeshData)
Copy link
Member

Choose a reason for hiding this comment

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

There is an instance here, where we could have regular text being rendered using SDF shader, when the regular less complex shader would suffice (if we have no sdf and the size of the text is to large and breaks batching)

perhaps we keep a single default mesh shader and a single sdf shader and we use the right one at render time?

@miltoncandelero
Copy link
Contributor Author

this is really nice work @miltoncandelero ! we have been looking to get this in for ages :D
left a comment, main thing is that I feel we should still keep the default mesh shader around for regular text if going through the if (distanceFieldType === 'none') path.

Check what "batchable= true" does. I think that is the default mesh Shader being used 🤔

@GoodBoyDigital
Copy link
Member

GoodBoyDigital commented Sep 13, 2021

this is really nice work @miltoncandelero ! we have been looking to get this in for ages :D
left a comment, main thing is that I feel we should still keep the default mesh shader around for regular text if going through the if (distanceFieldType === 'none') path.

Check what "batchable= true" does. I think that is the default mesh Shader being used 🤔

true! When batching, default shader is used. But batchable property is like a preference option, it is not guaranteed. When meshes get too large they are no longer batched. This can often be the case when it comes to bitmapText with lots of characters.

Batch check code inside mesh._render:

 if (
            shader.batchable
            && this.drawMode === DRAW_MODES.TRIANGLES
            && vertices.length < Mesh.BATCHABLE_SIZE * 2 <---- this could be false for large text
        )

@miltoncandelero
Copy link
Contributor Author

&& vertices.length < Mesh.BATCHABLE_SIZE * 2 <---- this could be false for large text

oh, you are right 😬

Ok, I will make it so that we keep meshes with the default shader and meshes with sdf 👍

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

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

amazing! love how you managed to leverage what we have and achieve this with such a little amount of code! Great job!

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Sep 20, 2021
@bigtimebuddy bigtimebuddy merged commit 67388c3 into pixijs:dev Sep 20, 2021
@bigtimebuddy
Copy link
Member

Great work @miltoncandelero 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants