-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
epaint: Memoize individual lines during text layout #5411
base: master
Are you sure you want to change the base?
Conversation
Preview available at https://egui-pr-preview.github.io/pr/5411-cache_galley_lines |
Okay, so I did the thing and implemented the Currently this "somewhat" works, I've managed to fix the selection issues I was seeing, but there is some layout issues you can actually see in the live demo above where the text is overlapping sometimes when wrapped (since this was present before the whole |
So I've managed to make this look pretty correct, these are the remaining issues:
|
I think I've made some good progress:
The remaining test failures are:
|
06e639e
to
139f286
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I haven't reviewed all of GalleyCache
yet, because it is a very dense and complex function.
I wonder how we best test and/or benchmark this though 🤔
I defiantly feel we should have a benchmark to justify this code, i.e. something that shows a big speedup before/after this PR.
// Note we **don't** ignore the leading/trailing whitespace here! | ||
row.size.x = target_max_x - target_min_x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? What does this affect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now that I think about it changing this comment was a mistake, leading and trailing whitespace is actually still ignored (I don't know why I thought otherwise). I'll change it back.
However ignoring whitespace here does seem kind of strange since the current docs for Row::rect
say that it includes whitespace. Wouldn't this be wrong after it passes through this function? Maybe at least that documentation could be adjusted to clarify this is only true for left-aligned non-justified text.
I also noticed a more important issue though I think, this function actually makes Row
s have glyphs that are outside their PlacedRow::pos
+ Row::size
rectangle (because it just repositions the glyphs and leaves the position unchanged). This currently breaks selecting such text. I'll try to fix this soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the issue found above and the comment. Although I still want to know what you think about changing the Row::size
comment, in general halign is surprising with how it suddenly totally changes how the coordinates work even though it makes perfect sense in hindsight.
crates/epaint/src/text/fonts.rs
Outdated
galley: galley.clone(), | ||
}); | ||
galley | ||
if job.break_on_newline { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should also only do this if the text is long enough and/or actually contains newline characters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we can check for a '\n', initially I was skeptical but the actual layout is way more expensive than this string find will cost us so it's probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the newline part, I think adding a length check might not be worth it because it'd definitely make testing harder if an issue "only showed up if the text is longer than 1000 characters".
crates/epaint/src/text/fonts.rs
Outdated
galley: galley.clone(), | ||
}); | ||
galley | ||
if job.break_on_newline { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment, let me know if you feel this is sufficient or if you had something else in mind.
while current != job.text.len() { | ||
let end = job.text[current..] | ||
.find('\n') | ||
.map_or(job.text.len(), |i| i + current + 1); | ||
let start = current; | ||
|
||
let mut line_job = LayoutJob { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This functions is very long.
I would love if we could break this up into parts, for instance:
let mut galleys = vec![];
for line_job in job.split_on_newlines() {
…
}
Galley::concat(galleys)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should such LayoutJob::split_on_newlines
and Galley::concat
functions be public API or do we want to keep them private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into splitting out these functions but it's actually non trivial, for example the splitting actually has to do the layout while it is doing the splitting because it has to update LayoutJob::max_rows
as more rows are being laid out. As for Galley::concat
, it needs access to pixels_per_point
for correct rounding so it'd be at the very least awkward as a public API (I guess you'd need to pass in Fonts
?) and as a private one it could accept FontsImpl
, it also needs access to the full LayoutJob
so it can store it inside the Galley
and also to check round_output_size_to_nearest_ui_point
.
I think we could do something like a Galley::concat(fonts: &mut FontsImpl, job: LayoutJob, galleys: impl Iterator<Item = Galley>) -> Galley
and keep splitting in GalleyCache::layout_multiline
or split it out into a method of GalleyCache
but it has to have access to GalleyCache
so it can't be on LayoutJob
. Also at that point I don't know whether scattering the implementation detail of FontsImpl
into a Galley
method is any more clean than keeping it in GalleyCache
.
Maybe another solution would be to actually disable this line caching if LayoutJob::max_rows
is finite, because in that scenario there's going to be more ways to invalidate the cached lines anyways and the performance benefits will be much more situational (any edit that will cause a line-count change will cause a re-layout of all subsequent lines). This could probably allow splitting out a LayoutJob::split_on_newlines
method.
I agree having a benchmark would be nice, currently the only thing I have is "it looks quicker" when you have a lot of text (deletion of small segments works instantly for example).
As for tests, I've encountered issues with selection drawing multiple times while working on this at this point, so maybe it would be beneficial if this was actually checked by the test suite, it looks like mouse events are supported in kittest so maybe that's a good start. Especially since I also think selection visuals might be another good place for optimization because currently selecting the whole text will force all It also looks like the |
f406701
to
40f237d
Compare
2ca2233
to
3e1ed18
Compare
I added a benchmark similar to the scenario I described above. |
This is an
almost completeimplementation of the approach described by emilk in this comment, excluding CoW semantics forLayoutJob
(but including them forRow
).It supersedes the previous unsuccessful attempt here: #4000.
Draft because:
Currently individual rows will haveends_with_newline
always set to false.This breaks selection with Ctrl+A (and probably many other things)
The whole block for doing the splitting and merging should probably become a function (I'll do that later).I haven't run the check script, the tests, and haven't made sure all of the examples build (although I assume they probably don't rely on Galley internals).Layout is sometimes incorrect (missing empty lines, wrapping sometimes makes text overlap).Row
s. Also this requires that we're fine making these very breaking changes.It does significantly improve the performance of rendering large blocks of text (if they have many newlines), this is the test program I used to test it (adapted from #3086):
code
I think the way to proceed would be to make a new type, something like
PositionedRow
, that would wrap anArc<Row>
but have a separatepos
and(that would meanends_with_newline
Row
only holds asize
instead of arect
). This type would of course have getters that would allow you to easily get aRect
from it and probably aDeref
to the underlyingRow
.I haven't done this yet because I wanted to get some opinions whether this would be an acceptable API first.This is now implemented, but of course I'm still open to discussion about this approach and whether it's what we want to do.Breaking changes (currently):
Galley::rows
field has a different type.PlacedRow
wrapper forRow
.Row
now uses a coordinate system relative to itself instead of theGalley
.TextEdit
#3086