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

Store UI render target info locally per node #17579

Merged
merged 95 commits into from
Feb 10, 2025

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Jan 28, 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 #17613
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.

Change `update_target_camera_system` to update the `ResolvedCameraCamera` components.
Renamed `NodeScaleFactor` -> `ResolvedScaleFactor`
@ickshonpe
Copy link
Contributor Author

ickshonpe commented Feb 8, 2025

Only comment is perhaps you could consolidate ComputedNodeTargetSize and ComputedNodeScaleFactor into one component.

Thanks for reviewing this. I agree, ran some benchmarks and I was definitely overestimating the benefits of more granular change detection. In practice changing the resolution or scale factor is almost always going to result in a full relayout of everything so there's no benefit to having the separate components.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is much more sensible. I've left a suggestion for improved docs for the new component.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 10, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 10, 2025
Merged via the queue into bevyengine:main with commit 300fe4d Feb 10, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Performance A change motivated by improving speed, memory usage or compile times C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
3 participants