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

Fix Canvas2D fonts performance #253

Conversation

wouterlucas
Copy link
Contributor

@wouterlucas wouterlucas commented Apr 22, 2024

This PR adds a "single page" Canvas2D text rendering path for small Canvas text usage along side of the original "scrollable" Canvas2D text path.

@wouterlucas wouterlucas added the performance Performance enhancement label Apr 22, 2024
forceFullLayoutCalc: false,
textW: 0,
textH: 0,
fontInfo: undefined,
fontFaceLoadedHandler: undefined,
isRenderable: false,
isScrollable: props.scrollable === true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to make this a state property? If a user were to change the scrollable property of a Text Node mid-lifecycle it would not have any affect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's exactly why. I figured it might be better to stay on the 3 canvasPage rendering instead of going back to the single, to avoid large discrepancies with rendering and positioning. If you feel that would be a better path, easy to flip it back to just state.props.scrollable and not have this here.

that said would this really be a usecase? a user repurposing a large chunk of scrollable text to a single non-scrollable piece of text?

@wouterlucas wouterlucas marked this pull request as ready for review May 9, 2024 21:10
@wouterlucas wouterlucas force-pushed the fix/canvas2d-fonts-performance branch from f1a8101 to 66a5056 Compare May 21, 2024 11:59
@wouterlucas wouterlucas force-pushed the fix/canvas2d-fonts-performance branch from 66a5056 to a651bd5 Compare May 22, 2024 08:58
@@ -160,6 +160,18 @@ export class ImageTexture extends Texture {
return `ImageTexture,${key},${resolvedProps.premultiplyAlpha ?? 'true'}`;
}

override free(): void {
if (this.props.src instanceof ImageData) {
// ImageData is a non-cacheable texture, so we need to free it manually
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect since the addition of the image factory/key PR #256

@frank-weindel
Copy link
Collaborator

Replaced by #319

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance enhancement
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants