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

ContentSize replacement fix #9753

Merged
merged 4 commits into from Sep 18, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Sep 10, 2023

Objective

If you remove a ContentSize component from a Bevy UI entity and then replace it ui_layout_system will remove the measure func from the internal Taffy layout tree but no new measure func will be generated to replace it since it's the widget systems that are responsible for creating their respective measure funcs not ui_layout_system. The widget systems only perform a measure func update on changes to a widget entity's content. This means that until its content is changed in some way, no content will be displayed by the node.

Example

This example spawns a text node which disappears after a few moments once its ContentSize component is replaced.

use bevy::prelude::*;
use bevy::ui::ContentSize;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, delayed_replacement)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(
        TextBundle::from_section(
            "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.",
            TextStyle::default(),
        )
    );
}

// Waits a few frames to make sure the font is loaded and the text's glyph layout has been generated.
fn delayed_replacement(mut commands: Commands, mut count: Local<usize>, query: Query<Entity, With<Style>>) {
    *count += 1;
    if *count == 10 {
        for item in query.iter() {
            commands
                .entity(item)
                .remove::<ContentSize>()
                .insert(ContentSize::default());
        }
    }
}

Solution

Perform ui_layout_system's ContentSize removal detection and resolution first, before the measure func updates.
Then in the widget systems, generate a new Measure when a ContentSize component is added to a widget entity.

Changelog

  • measure_text_system, update_image_content_size_system and update_atlas_content_size_system generate a new Measure when a ContentSize component is added.

If you remove a `ContentSize` component from a Bevy UI entity and then immediately replace it `ui_layout_system` will remove the measure func from the internal Taffy layout tree but no new measure func will be generated to replace it since it's the widget systems that are responsible for creating their respective measure funcs not `ui_layout_system`. The widget systems only perform updates on changes to their content, and don't check `ContentSize` for changes. This means that until the content is changed in some way, no content will be displayed by the node.

This commit fixes this by performing `ui_layout_system`'s `ContentSize` removal detection and resolution first, before measure func updates and in the widget systems generating a new `Measure` when a  `ContentSize` component is added to a widget entity.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Sep 10, 2023
@rparrett
Copy link
Contributor

rparrett commented Sep 14, 2023

If you remove a ContentSize component from a Bevy UI entity and then replace it

Just for the sake of documentation:

  • Is ContentSize ever removed in Bevy / does this fix a bug higher up somewhere?
  • What's the use case for a user doing this?

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Sep 14, 2023

If you remove a ContentSize component from a Bevy UI entity and then replace it

Just for the sake of documentation:

  • Is ContentSize ever removed in Bevy / does this fix a bug higher up somewhere?
  • What's the use case for a user doing this?

I'm not sure if there are any specific use cases. @viridia seemed to be experimenting with some quite complicated ideas that might require it.

It's about the API obeying the basic principle that an entity is defined by its components.

For example, suppose that you have an entity with the TextBundle components that you are using to display some text. The ideal is that if you remove the TextBundle components and then immediately insert the SpriteBundle components then that entity is now a sprite, the text is gone, you see a sprite instead, there are no panics and nothing weird happens.

@viridia
Copy link
Contributor

viridia commented Sep 14, 2023

I haven't gotten around to messing with ContentSize yet. However...

I am working with adding and removing components. For example, suppose you have a style asset that has a "background-color" attribute. The background color can either be a color (in which case the BackgroundColor component needs to exist) or it can be "transparent", which means "don't render a background color at all" (it does not mean render with alpha 0), which requires that the BackgroundColor component be removed from the entity.

This style property can change due to several causes: a hot reload of the style asset; an override of a context variable farther up the tree; a dynamic style change due to a state change in a widget.

That being said, it's hard to imagine a use case for dynamically changing ContentSize, other than hot reloading of a style asset.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

while a very uncommon case, this should have pretty much no perf impact when not hitting that case

@mockersf mockersf 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 Sep 17, 2023
@alice-i-cecile
Copy link
Member

@ickshonpe once merge conflicts are fixed I'll merge this for you.

@mockersf mockersf added this pull request to the merge queue Sep 18, 2023
Merged via the queue into bevyengine:main with commit dc124ee Sep 18, 2023
21 checks passed
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

If you remove a `ContentSize` component from a Bevy UI entity and then
replace it `ui_layout_system` will remove the measure func from the
internal Taffy layout tree but no new measure func will be generated to
replace it since it's the widget systems that are responsible for
creating their respective measure funcs not `ui_layout_system`. The
widget systems only perform a measure func update on changes to a widget
entity's content. This means that until its content is changed in some
way, no content will be displayed by the node.

### Example

This example spawns a text node which disappears after a few moments
once its `ContentSize` component is replaced.

```rust
use bevy::prelude::*;
use bevy::ui::ContentSize;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, delayed_replacement)
        .run();
}

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());
    commands.spawn(
        TextBundle::from_section(
            "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.",
            TextStyle::default(),
        )
    );
}

// Waits a few frames to make sure the font is loaded and the text's glyph layout has been generated.
fn delayed_replacement(mut commands: Commands, mut count: Local<usize>, query: Query<Entity, With<Style>>) {
    *count += 1;
    if *count == 10 {
        for item in query.iter() {
            commands
                .entity(item)
                .remove::<ContentSize>()
                .insert(ContentSize::default());
        }
    }
}
```

## Solution

Perform `ui_layout_system`'s `ContentSize` removal detection and
resolution first, before the measure func updates.
Then in the widget systems, generate a new `Measure` when a
`ContentSize` component is added to a widget entity.

## Changelog

* `measure_text_system`, `update_image_content_size_system` and
`update_atlas_content_size_system` generate a new `Measure` when a
`ContentSize` component is added.
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-Bug An unexpected or incorrect behavior 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.

None yet

5 participants