Replace the local text queues in the text systems with flags stored in a component #8549

ickshonpe · 2023-05-05T13:38:58Z

Objective

text_system and measure_text_system both keep local queues to keep track of text node entities that need recomputations/remeasurement, which scales very badly with large numbers of text entities (O(n^2)) and makes the code quite difficult to understand.

Also text_system filters for Changed<Text>, this isn't something that it should do. When a text node entity fails to be processed by measure_text_system because a font can't be found, the text node will still be added to text_system's local queue for recomputation. Text should only ever be queued by text_system when a text node's geometry is modified or a new measure is added.

Solution

Remove the local text queues and use a component TextFlags to schedule remeasurements and recomputations.

Changelog

Created a component TextFlags with fields remeasure and recompute, which can be used to schedule a text remeasure or recomputation respectively and added it to TextBundle.
Removed the local text queues from measure_text_system and text_system and instead use the TextFlags component to schedule remeasurements and recomputations.

Migration Guide

The component TextFlags has been added to TextBundle.

…s(&entity)` which just add all unqueued text. Fixed it so that only changed and unqueued text nodes are added to the queue.

…re_text_system` and `text_system`

* Created a new component `TextFlags` that tracks text node status. * Replaced the text queue in `measure_text_system` with a system using the `TextFlags` component to track unmeasured text nodes.

* Added the `TextFlags` component to `TextBundle`.

…onent to schedule text computations.

* removed unused imports

…o functions

ickshonpe · 2023-05-05T13:42:01Z

Big gains with lots of text nodes:

cargo run --example many_buttons --profile stress-test --features trace_tracy -- recompute-text

crates/bevy_ui/src/widget/text.rs

alice-i-cecile

This is a much nicer implementation, and the performance benefits are non-trivial.

ickshonpe · 2023-05-05T15:42:35Z

This is a much nicer implementation, and the performance benefits are non-trivial.

Yep, really pleased with how this PR turned out. I'd rather not have added yet another UI component but there isn't really anywhere to put the flag variables that makes sense and doesn't get filtered for changes.

nicoburns

One comment on terminology. Otherwise looks good to me.

nicoburns · 2023-05-07T09:55:52Z

crates/bevy_ui/src/widget/text.rs

+pub struct TextFlags {
+    /// create a new measure for the text
+    remeasure: bool,
+    /// recompute the text
+    recompute: bool,
+}


I feel like remeasure should be called regenerate_measure_function or similar. "measuring" is when you call the measure function not when you generate it. recompute could then be called remeasure, but I think recompute is also fine.

Yeah the terminology everywhere needs to be tightened up. I've been loose with it because I'm not really
clear on what's going to be intuitive for users etc, so just used short easy names. Even regenerate_measure_function is a bit misleading, since the flag is most commonly going to be used when a new text node entity is spawned but its fonts haven't loaded, so it is rescheduling the initial creation of a measure function until the next frame, not regenerating an existing measure function.

Changed it to generate_measure_function, seems okay.

Thought about this again, and I think these renamings are better as they capture the correct semantics:

generate_measure_func -> needs_new_measure_func

recompute -> needs_recompute

* `generate_measure_func` -> `needs_new_measure_func` * `recompute` -> `needs_recompute`

ickshonpe · 2023-05-08T09:13:18Z

It's outside of the scope of this PR and I don't want to add any more changes, but it seems like it should be impossible for queue_text to return a NoSuchFont error since the fonts need to be already loaded to create a measure func. It should probably panic on this error, and there might be room for some other simplifications somewhere.

ickshonpe added 17 commits May 5, 2023 08:43

Use a single query instead of a ParamSet for measure_text_system

a8a820c

Replaced the ParamSet parameter with a single query in text_system.

dfca89c

cargo fmt --all

4e1ade4

The change detection predicate was checking for `!queued_text.contain…

9a424c7

…s(&entity)` which just add all unqueued text. Fixed it so that only changed and unqueued text nodes are added to the queue.

removed left over debug variable

b0620d0

Renamed the Local parameter queued_text to text_queue in `measu…

686e109

…re_text_system` and `text_system`

fixed imports

baece54

Renamed new_queue to new_text_queue.

d69d8ca

changes:

6e93578

* Created a new component `TextFlags` that tracks text node status. * Replaced the text queue in `measure_text_system` with a system using the `TextFlags` component to track unmeasured text nodes.

* Added check to filter unchanged Text in measure_text_system.

85eb848

* Added the `TextFlags` component to `TextBundle`.

Remove the text queue from text_system and use the TextFlags comp…

7309bac

…onent to schedule text computations.

* cargo fmt --all,

de73323

* removed unused imports

Don't recompute text that needs a remeasure.

90427f2

cargo fmy

aca22ab

fixed text flags for full remeasure

253ec5d

Moved duplicate code from measure_text_system and text_system int…

05eaf9d

…o functions

formatting

396a59c

alice-i-cecile added C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Bug An unexpected or incorrect behavior labels May 5, 2023

alice-i-cecile reviewed May 5, 2023

View reviewed changes

crates/bevy_ui/src/widget/text.rs Show resolved Hide resolved

alice-i-cecile approved these changes May 5, 2023

View reviewed changes

alice-i-cecile added this to the 0.11 milestone May 5, 2023

ickshonpe added 3 commits May 5, 2023 14:51

Added TextFlags doc comments explaining it is for internal use.

b0aa6d9

cargo fmt

dc3e846

Fixes for too_many_arguments and needless_borrow lints.

9c2f299

ickshonpe mentioned this pull request May 5, 2023

Simplify text system queries #8547

Closed

alice-i-cecile requested a review from nicoburns May 5, 2023 15:43

nicoburns suggested changes May 7, 2023

View reviewed changes

Renamed the remeasure field of TextFlags to generate_measure_func

a4a3f65

alice-i-cecile requested a review from nicoburns May 7, 2023 18:37

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 7, 2023

Renamings for TextFlags fields:

caf6c79

* `generate_measure_func` -> `needs_new_measure_func` * `recompute` -> `needs_recompute`

nicoburns approved these changes May 8, 2023

View reviewed changes

alice-i-cecile added this pull request to the merge queue May 8, 2023

Merged via the queue into bevyengine:main with commit e0a94ab May 8, 2023

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

Selene-Amanita added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the local text queues in the text systems with flags stored in a component #8549

Replace the local text queues in the text systems with flags stored in a component #8549

ickshonpe commented May 5, 2023 •

edited

Loading

ickshonpe commented May 5, 2023 •

edited

Loading

alice-i-cecile left a comment

ickshonpe commented May 5, 2023

nicoburns left a comment

nicoburns May 7, 2023

ickshonpe May 7, 2023 •

edited

Loading

ickshonpe May 7, 2023

ickshonpe May 8, 2023 •

edited

Loading

ickshonpe commented May 8, 2023 •

edited

Loading

Replace the local text queues in the text systems with flags stored in a component #8549

Replace the local text queues in the text systems with flags stored in a component #8549

Conversation

ickshonpe commented May 5, 2023 • edited Loading

Objective

Solution

Changelog

Migration Guide

ickshonpe commented May 5, 2023 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

ickshonpe commented May 5, 2023

nicoburns left a comment

Choose a reason for hiding this comment

nicoburns May 7, 2023

Choose a reason for hiding this comment

ickshonpe May 7, 2023 • edited Loading

Choose a reason for hiding this comment

ickshonpe May 7, 2023

Choose a reason for hiding this comment

ickshonpe May 8, 2023 • edited Loading

Choose a reason for hiding this comment

ickshonpe commented May 8, 2023 • edited Loading

ickshonpe commented May 5, 2023 •

edited

Loading

ickshonpe commented May 5, 2023 •

edited

Loading

ickshonpe May 7, 2023 •

edited

Loading

ickshonpe May 8, 2023 •

edited

Loading

ickshonpe commented May 8, 2023 •

edited

Loading