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

Add an MSDF-based text rendering sample #382

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Add an MSDF-based text rendering sample #382

merged 2 commits into from
Mar 8, 2024

Conversation

toji
Copy link
Contributor

@toji toji commented Mar 8, 2024

This sample has been languishing in a branch for a while because react's handling of asset files prevented it from working correctly. Now that react has been removed from the sample pages, though, it works as expected! Thanks @greggman!

Updated to match the newer sample format. Please let me know if I missed anything, but it looks like everything is working as expected when I run it locally.

@toji toji requested a review from greggman March 8, 2024 18:21
sample/textRenderingMsdf/main.ts Outdated Show resolved Hide resolved
sample/textRenderingMsdf/main.ts Outdated Show resolved Hide resolved
sample/textRenderingMsdf/main.ts Outdated Show resolved Hide resolved
{
view: undefined, // Assigned later

clearValue: { r: 0.0, g: 0.0, b: 0.0, a: 1.0 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Do you think we should prefer [0, 0, 0, 1] do you prefer this longer format? My impression is maybe this longer format was the only format originally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A lot of this was just copied from an earlier version of the rotating cube sample, but I'm happy to update it as I go! My preference has always been for the more concise array format if I'm typing it directly. I've hit a couple of places where it's made more sense to use the longer dictionary versions, but that's almost always when processing some sort of externally provided values that are already named.

text looks smoother while using less memory than the Canvas 2D approach, especially at high
zoom levels. They can be used to render larger amounts of text efficiently.

The font texture is generated using <a href="https://msdf-bmfont.donmccurdy.com/">Don
Copy link
Collaborator

Choose a reason for hiding this comment

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

Up to you but this is markdown so maybe markdown format [Don McCurdy's MSDF font generation tool](https://msdf-bmfont.donmccurdy.com/). I only suggest that because I don't want to encourage arbitrary HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use markdown, though it threw me off for a second because I had the text indented in the backticks, so it read them as a literal. The indented text was then treated as a code block, and didn't format the link correctly. Easy to fix once I realized what was going on.

sample/textRenderingMsdf/msdfText.wgsl Outdated Show resolved Hide resolved
@greggman
Copy link
Collaborator

greggman commented Mar 8, 2024

oh, and you might want to npm run fix

toji added 2 commits March 8, 2024 13:29
This sample has been languishing in a branch for a while because
react's handling of asset files prevented it from working correctly.
Now that react has been removed from the sample pages, though, it
works as expected! Thanks @greggman!
@toji toji force-pushed the text-rendering-msdf branch from 912220a to 3c77902 Compare March 8, 2024 21:38
@toji
Copy link
Contributor Author

toji commented Mar 8, 2024

Thanks for the feedback! Updated everything, rebased, and cleaned up a few additional TypeScript complaints as well.

@toji toji merged commit ad7d067 into main Mar 8, 2024
1 check passed
@toji toji deleted the text-rendering-msdf branch March 8, 2024 21:49
@@ -0,0 +1,18 @@
export default {
name: 'Text Rendering - MSDF',
description: `This example uses multichannel signed distance fields (MSDF) to render text. MSDF
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm guessing you already know this but just in case... you can do this if you want

{
  description: `\
This example uses multichannel signed distance fields (MSDF) to render text. MSDF
fonts are more complex to implement than using Canvas 2D to generate text, but the resulting
text looks smoother while using less memory than the Canvas 2D approach, especially at high
zoom levels. They can be used to render larger amounts of text efficiently.`,
}

not suggesting you change anything. Just passing it on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should trim the description if we don't already, then you don't have to worry about the backslash. It also allows putting the closing `, on a separate line. This is what we do in test descriptions in CTS.

Copy link
Collaborator

@greggman greggman Mar 11, 2024

Choose a reason for hiding this comment

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

If trim means just description.trim() I'm for it but it wouldn't have helped the original issue here which was a description like this.

{
  description: `
     This is the description
     bla bla bla bla bla bla
     bla bla bla bla bla bla
     bla bla bla bla bla bla
  `
}

Because that leading space is sensitive in markdown. We could try to trim every line the same amount that feels like overkill and likely to break. I think just knowing the text is markdown and knowing markdown is space sensitive is enough. Maybe we should rename description to descriptionMarkdown. A simple trim just for leading and training space though is unlikely to break anything. It's unlikely the first line would be indented intentionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants