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

measure_text_system scale factor change detection watches the camera target's scale factor, not the text node's #17578

Closed
ickshonpe opened this issue Jan 28, 2025 · 0 comments · Fixed by #17579
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@ickshonpe
Copy link
Contributor

ickshonpe commented Jan 28, 2025

Bevy version

main

What's wrong

measure_text_system queries for the camera entity of a text node and then using the camera entity looks up the camera target's scale factor. Then to detect if the scale factor has changed it looks up the camera's scale factor from the previous frame it keeps stored in a Local and compares them. But if the text node's target camera was changed since the previous frame it will be comparing the wrong scale factors and the text node's measure func won't be updated if its scale factor changed.

@ickshonpe ickshonpe added A-Text Rendering and layout for characters C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets labels Jan 28, 2025
github-merge-queue bot pushed a commit that referenced this issue Feb 10, 2025
# Objective

It's difficult to understand or make changes to the UI systems because
of how each system needs to individually track changes to scale factor,
windows and camera targets in local hashmaps, particularly for new
contributors. Any major change inevitably introduces new scale factor
bugs.

Instead of per-system resolution we can resolve the camera target info
for all UI nodes in a system at the start of `PostUpdate` and then store
it per-node in components that can be queried with change detection.

Fixes #17578
Fixes #15143

## Solution

Store the UI render target's data locally per node in a component that
is updated in `PostUpdate` before any other UI systems run.

This component can be then be queried with change detection so that UI
systems no longer need to have knowledge of cameras and windows and
don't require fragile custom change detection solutions using local
hashmaps.

## Showcase
Compare `measure_text_system` from main (which has a bug the causes it
to use the wrong scale factor when a node's camera target changes):
```
pub fn measure_text_system(
    mut scale_factors_buffer: Local<EntityHashMap<f32>>,
    mut last_scale_factors: Local<EntityHashMap<f32>>,
    fonts: Res<Assets<Font>>,
    camera_query: Query<(Entity, &Camera)>,
    default_ui_camera: DefaultUiCamera,
    ui_scale: Res<UiScale>,
    mut text_query: Query<
        (
            Entity,
            Ref<TextLayout>,
            &mut ContentSize,
            &mut TextNodeFlags,
            &mut ComputedTextBlock,
            Option<&UiTargetCamera>,
        ),
        With<Node>,
    >,
    mut text_reader: TextUiReader,
    mut text_pipeline: ResMut<TextPipeline>,
    mut font_system: ResMut<CosmicFontSystem>,
) {
    scale_factors_buffer.clear();

    let default_camera_entity = default_ui_camera.get();

    for (entity, block, content_size, text_flags, computed, maybe_camera) in &mut text_query {
        let Some(camera_entity) = maybe_camera
            .map(UiTargetCamera::entity)
            .or(default_camera_entity)
        else {
            continue;
        };
        let scale_factor = match scale_factors_buffer.entry(camera_entity) {
            Entry::Occupied(entry) => *entry.get(),
            Entry::Vacant(entry) => *entry.insert(
                camera_query
                    .get(camera_entity)
                    .ok()
                    .and_then(|(_, c)| c.target_scaling_factor())
                    .unwrap_or(1.0)
                    * ui_scale.0,
            ),
        };

        if last_scale_factors.get(&camera_entity) != Some(&scale_factor)
            || computed.needs_rerender()
            || text_flags.needs_measure_fn
            || content_size.is_added()
        {
            create_text_measure(
                entity,
                &fonts,
                scale_factor.into(),
                text_reader.iter(entity),
                block,
                &mut text_pipeline,
                content_size,
                text_flags,
                computed,
                &mut font_system,
            );
        }
    }
    core::mem::swap(&mut *last_scale_factors, &mut *scale_factors_buffer);
}
```

with `measure_text_system` from this PR (which always uses the correct
scale factor):
```
pub fn measure_text_system(
    fonts: Res<Assets<Font>>,
    mut text_query: Query<
        (
            Entity,
            Ref<TextLayout>,
            &mut ContentSize,
            &mut TextNodeFlags,
            &mut ComputedTextBlock,
            Ref<ComputedNodeTarget>,
        ),
        With<Node>,
    >,
    mut text_reader: TextUiReader,
    mut text_pipeline: ResMut<TextPipeline>,
    mut font_system: ResMut<CosmicFontSystem>,
) {
    for (entity, block, content_size, text_flags, computed, computed_target) in &mut text_query {
        // Note: the ComputedTextBlock::needs_rerender bool is cleared in create_text_measure().
        if computed_target.is_changed()
            || computed.needs_rerender()
            || text_flags.needs_measure_fn
            || content_size.is_added()
        {
            create_text_measure(
                entity,
                &fonts,
                computed_target.scale_factor.into(),
                text_reader.iter(entity),
                block,
                &mut text_pipeline,
                content_size,
                text_flags,
                computed,
                &mut font_system,
            );
        }
    }
}
```

## Testing

I removed an alarming number of tests from the `layout` module but they
were mostly to do with the deleted camera synchronisation logic. The
remaining tests should all pass now.

The most relevant examples are `multiple_windows` and `split_screen`,
the behaviour of both should be unchanged from main.

---------

Co-authored-by: UkoeHB <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Text Rendering and layout for characters A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant