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

[Impeller] Share LazyGlyphAtlas across EntityPass #38244

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

luckysmg
Copy link
Contributor

@luckysmg luckysmg commented Dec 13, 2022

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#116815

Now, every EntityPass will create a single LazyGlyphAtlas to use in Canvas::DrawTextFrame, but it seems that there is no need for creating a LazyGlyphAtlas for per EntityPass, and I think maybe we can create a single LazyGlyphAtlas to reuse it across EntityPass.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@luckysmg luckysmg marked this pull request as draft Dec 13, 2022
@luckysmg luckysmg marked this pull request as ready for review Dec 13, 2022
@luckysmg luckysmg changed the title Reuse LazyGlyphAtlas [Impeller] Reuse LazyGlyphAtlas Dec 13, 2022
@luckysmg luckysmg requested a review from chinmaygarde Dec 13, 2022
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

I'm not sure that I understand the change that this PR is trying to make.

LazyGlyphAtlas is attached to the entity pass allowing it to be used across multiple text content canvas operations. Moving it to the canvas makes it harder to reuse.

@luckysmg
Copy link
Contributor Author

luckysmg commented Dec 14, 2022

Hi @jonahwilliams
Now, per EntityPass will create a LazyGlyphAtlas. (If there are two EntityPass, will create two LazyGlyphAtlas).
If we move it to canvas, multiple EntityPass can reuse this single LazyGlyphAtlas. LMK if I missed something O(∩_∩)O

@jonahwilliams
Copy link
Member

Oh I see, sorry I misunderstood. yes that will accomplish reuse.

We're going to end up with a few more objects that can be shared across entity pass, the tesselator to start with, and at some point some sort of geometry cache too. Is canvas.cc a reasonable place to put these for now, or should we create a separate class to hold them @chinmaygarde ?

@luckysmg luckysmg changed the title [Impeller] Reuse LazyGlyphAtlas [Impeller] Share LazyGlyphAtlas across EntityPass Dec 16, 2022
@luckysmg luckysmg requested a review from jonahwilliams Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ⚙️ In Progress
3 participants