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

text_system split #7779

Merged
merged 48 commits into from
Apr 17, 2023
Merged

text_system split #7779

merged 48 commits into from
Apr 17, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Feb 21, 2023

Objective

text_system runs before the UI layout is calculated and the size of the text node is determined, so it cannot correctly shape the text to fit the layout, and has no way of determining if the text needs to be wrapped.

The function text_constraint attempts to determine the size of the node from the local size constraints in the Style component. It can't be made to work, you have to compute the whole layout to get the correct size. A simple example of where this fails completely is a text node set to stretch to fill the empty space adjacent to a node with size constraints set to Val::Percent(50.). The text node will take up half the space, even though its size constraints are Val::Auto

Also because the text_system queries for changes to the Style component, when a style value is changed that doesn't affect the node's geometry the text is recomputed unnecessarily.

Querying on changes to Node is not much better. The UI layout is changed to fit the CalculatedSize of the text, so the size of the node is changed and so the text and UI layout get recalculated multiple times from a single change to a Text.

Also, the MeasureFunc doesn't work at all, it doesn't have enough information to fit the text correctly and makes no attempt.

Fixes #7663, #6717, #5834, #1490,

Solution

Split the text_system into two functions:

  • measure_text_system which calculates the size constraints for the text node and runs before UiSystem::Flex
  • text_system which runs after UiSystem::Flex and generates the actual text.
  • Fix the MeasureFunc calculations.

Text wrapping in main:
Capturemain

With this PR:
captured_wrap

Changelog

  • Removed the previous fields from CalculatedSize. CalculatedSize now contains a boxed Measure.
  • Added measurement module to bevy_ui.
  • Added the method create_text_measure to TextPipeline.
  • Added a new system measure_text_system that runs before UiSystem::Flex that creates a MeasureFunc for the text.
  • Rescheduled text_system to run after UiSystem::Flex.
  • Added a trait Measure. A Measure is used to compute the size of a UI node when the size of that node is based on its content.
  • Added ImageMeasure and TextMeasure which implement Measure.
  • Added a new component UiImageSize which is used by update_image_calculated_size_system to track image size changes.
  • Added a UiImageSize component to ImageBundle.

Migration Guide

ImageBundle has a new component UiImageSize which contains the size of the image bundle's texture and is updated automatically by update_image_calculated_size_system

* Added a new trait `MeasureNode`.
* Added new structs `ImageMeasure` and `BasicMeasure` that implement `MeasureNode`.
* Add a field to `CalculatedSize` called `measure` that takes a boxed `MeasureNode`.
* `upsert_leaf` uses the `measure` of `CalculatedSize` to create a `MeasureFunc` for the node.
    * Added the `TextLayoutInfo` component to `TextBundle`.
    * Added the `TextLayoutInfo` component to `Text2dBundle`.
    * Changed `TextLayoutInfo` queries to be non-optional.
* Renamed `CalculatedSize` to `IntrinsicSize`. It is now non-copy. Added a field `measure` and removed `preserve_aspect_ratio`.
* Added `measurement` module to `bevy_ui`
* Added `Measure` trait. A `Measure` is used to compute the size of an intrisically sized node.
* Added `ImageMeasure` and `FixedMeasure` `Measure` implementations.
* Changed `update_image_calculated_size_system` to use `ImageMeasure`.
* Changed `upsert_leaf` to use the `Measure` of `CalculatedSize` for the `MeasureFunc` of intrinsically sized nodes.
* Added the system (dummy atm) `measure_text_system`
* Changed the system execution order so that `measure_text_system` replaces `text_system` in the order.
`text_system` now runs after `UiSystem::Flex`.

Previously the `text_system` ran before the layout was calculated and the size of the text node was determined,
so it couldn't shape the text correctly to fit the layout, and had no way of determining if the text needed to be wrapped.
There was a hack, the system `text_constraint` that tried to determine the size of the node from the local size constraints of the node in its `Style` component.
This could not work correctly, `Val::Percent` constraints just had to be ignored as they are calcualted from size of the parent node and the `Val::Px` constraints are just a guess,
without computing the rest of the layout.
Also because the `text_system` queried for changes to the `Style` component, and not the `Node` component, it couldn't react to changes in the layout correctly.
The layout system then wouldn't recieve all the information it would need to fit the text node correctly, such as the `max-content` and `min-content` sizes.
* implemented the `measure_text_system` function.
* Added the `TextMeasure` type that implements measure.
* Added the `TextQueue` resource struct.
* Added methods to the `TextPipeline` to get min and max content sizes for the text.
changes:
* Removed `TextQueue`
* Added `min_content`, `max_content` and `ideal` fields to IntrinsicSize.
* Added `ideal_height` field to `TextMeasure`.
* Fixed text system queueing and change detection issues. `measure_text_system` only queries for modified `Text`,
`text_system` queuries for `Text` or `Node` changes.
@ickshonpe ickshonpe changed the title text system split text_system split Feb 21, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 21, 2023
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Feb 21, 2023
@alice-i-cecile
Copy link
Member

CC @TimJentzsch @nicoburns for review.

@nicoburns
Copy link
Contributor

I think the text node's dimensions need to computed inside the measure function, because the measure function needs to be able to answer the question "Given that your width is X, what should your height be?". That can't be precomputed.

The way I'd imagine this working is something like:

  • Glyph shaping and determining potential wrap points (e.g. via word segmentation) happens in a separate system prior to layout computation. This should be cached/memoized such that it is only recomputed at most once per update to the text.
  • One of the outputs of the above process should be something like a Vec<f32> where each item in the Vec represents the length of a word (or perhaps glyph depending on the wrapping mode).
  • This Vec<f32> should be captured by the measure func
  • The measure func then uses it to compute the correct size given the input constraints every time it's called by packing words into lines (similar to the measure function used in Taffy's generated tests). This could be further cached (esp. the min-content and max-content size) but I would expect performance to be acceptable even if it's not.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Feb 21, 2023

I think the text node's dimensions need to computed inside the measure function, because the measure function needs to be able to answer the question "Given that your width is X, what should your height be?". That can't be precomputed.

The way I'd imagine this working is something like:

  • Glyph shaping and determining potential wrap points (e.g. via word segmentation) happens in a separate system prior to layout computation. This should be cached/memoized such that it is only recomputed at most once per update to the text.
  • One of the outputs of the above process should be something like a Vec<f32> where each item in the Vec represents the length of a word (or perhaps glyph depending on the wrapping mode).
  • This Vec<f32> should be captured by the measure func
  • The measure func then uses it to compute the correct size given the input constraints every time it's called by packing words into lines (similar to the measure function used in Taffy's generated tests). This could be further cached (esp. the min-content and max-content size) but I would expect performance to be acceptable even if it's not.

Oh no. I was trying to do it that way, precalculate the glyph geometry, and then do the layout calculations in the measurefunc except (for no reason at all) I believed I'd still need access to the TextPipeline and Font data from inside the measurefunc 😅. Instead, I've got this terrible hack where I compute the layout initially with the min_content height, generate the text to fit the returned width and then recompute the layout again with the height of the generated text.

That simplifies things enormously, should be able to clean up everything now.

Copy link
Contributor

@Weibye Weibye left a comment

Choose a reason for hiding this comment

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

  1. This is excellent effort!
  2. I do agree with Nicoburns on the implementation design.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Apr 16, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 17, 2023
Merged via the queue into bevyengine:main with commit ffc62c1 Apr 17, 2023
shnarazk added a commit to shnarazk/nhk_now that referenced this pull request Apr 17, 2023
alice-i-cecile pushed a commit that referenced this pull request Apr 17, 2023
# Objective

Followup to #7779 which tweaks the actual text measurement algorithm to
be more robust.

Before:

<img width="822" alt="Screenshot 2023-04-17 at 18 12 05"
src="https://user-images.githubusercontent.com/1007307/232566858-3d3f0fd5-f3d4-400a-8371-3c2a3f541e56.png">

After:

<img width="810" alt="Screenshot 2023-04-17 at 18 41 40"
src="https://user-images.githubusercontent.com/1007307/232566919-4254cbfa-1cc3-4ea7-91ed-8ca1b759bacf.png">

(note extra space taken up in header in before example)

## Solution

- Text layout of horizontal text (currently the only kind of text we
support) is now based solely on the layout constraints in the horizontal
axis. It ignores constraints in the vertical axis and computes vertical
size based on wrapping subject to the horizontal axis constraints.
- I've also added a paragraph to the `grid` example for testing / demo
purposes.
shnarazk added a commit to shnarazk/nhk_now that referenced this pull request Apr 17, 2023
* a snapshot

* a snapshot with bugs

* no compile errors

* an attempt

* another snapshot still buggy

* clean up

* a snapshot

* use loop for building components

* tiny tweaks

* switch to Bevy
- modified:   Cargo.toml
- new file:   assets/fonts/NotoSansCJKjp-Regular.otf
- modified:   src/main.rs

* modified:   Cargo.toml; modified:   src/main.rs

* add button from examples/ui/button.rs

* add async stuff

* add async data fetcher

* tiny changes

* add some memo

* change labels to buttons

* desktop mode; fix some glitches

* add some interaction

* switch to reqwest

* use a modified copy of https://github.com/TotalKrill/bevy_mod_reqwest

* clean up

* written massively

* add Timeline component

* refactor Components and Resources

* update display with recieved data

* UI

* cargo clippy:   src/reqwest_plugin.rs

* try to make contents wider and wrapped

* revise layout settings

* editorial design

* more UI tweaks

* ditto

* UI tweaks and refactoring

* modified:   Cargo.toml

* adapt to auto-reflow text by bevyengine/bevy#7779 (comment)

* mention https://github.com/TotalKrill/bevy_mod_reqwest
@ickshonpe ickshonpe mentioned this pull request Jun 24, 2023
github-merge-queue bot pushed a commit that referenced this pull request Jun 26, 2023
# Objective

In Bevy 10.1 and before, the only way to enable text wrapping was to set
a local `Val::Px` width constraint on the text node itself.
`Val::Percent` constraints and constraints on the text node's ancestors
did nothing.

#7779 fixed those problems. But perversely displaying unwrapped text is
really difficult now, and requires users to nest each `TextBundle` in a
`NodeBundle` and apply `min_width` and `max_width` constraints. Some
constructions may even need more than one layer of nesting. I've seen
several people already who have really struggled with this when porting
their projects to main in advance of 0.11.

## Solution

Add a `NoWrap` variant to the `BreakLineOn` enum.
If `NoWrap` is set, ignore any constraints on the width for the text and
call `TextPipeline::queue_text` with a width bound of `f32::INFINITY`.



---

## Changelog
* Added a `NoWrap` variant to the `BreakLineOn` enum.
* If `NoWrap` is set, any constraints on the width for the text are
ignored and `TextPipeline::queue_text` is called with a width bound of
`f32::INFINITY`.
* Changed the `size` field of `FixedMeasure` to `pub`. This shouldn't
have been private, it was always intended to have `pub` visibility.
* Added a `with_no_wrap` method to `TextBundle`.

## Migration Guide

`bevy_text::text::BreakLineOn` has a new variant `NoWrap` that disables
text wrapping for the `Text`.
Text wrapping can also be disabled using the `with_no_wrap` method of
`TextBundle`.
@Selene-Amanita 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with the text system, change detection and MeasureFunc
7 participants