Fix UI surface rounding

[jorgeandrecastro]

Objective

Fix a logic bug in UiSurface::get_layout where the global Taffy rounding state was unconditionally re-enabled at the end of the function, regardless of the use_rounding parameter passed by the caller.

Solution

Modified UiSurface::get_layout to conditionally restore the rounding state (self.taffy.enable_rounding()) only if use_rounding was set to true, preventing unintended state overrides.

Testing
Ran unit tests specifically for the UI module using

 cargo test -p bevy_ui --lib

ensuring all tests pass successfully.
Tested locally on Linux.

[alice-i-cecile]

This needs a comment; why are we temporarily disabling rounding?

[ickshonpe]

Taffy's API is a bit confusing here, which is why this bug crept in. enable/disable_rounding does two things. It enables or disables whether the rounded final_layout is updated during layout. But also after the layout is generated it controls whether the per node layout getter function returns rounded or unrounded geometry:

pub fn layout(&self, node: NodeId) -> TaffyResult<&Layout> {
        if self.config.use_rounding {
            Ok(&self.nodes[node.into()].final_layout)
        } else {
            Ok(&self.nodes[node.into()].unrounded_layout)
        }
    }

[jorgeandrecastro]

This needs a comment; why are we temporarily disabling rounding?

Hello alice-i-cecille 😅 thanks for the suggestion I've added a comment to document this behavior as requested, well why we need to temporarily disable rounding? It's because Taffy's enable/disable_rounding setting controls both how geometry is generated and what the per-node layout getter returns. We temporarily disable it to fetch the unrounded size, and then restore the caller's original use_rounding preference.

I found this "issue " , cause a was working on another one before so i saw this and i pull request, Hope its helps bevy engine 😀

[jorgeandrecastro]

Taffy's API is a bit confusing here, which is why this bug crept in. enable/disable_rounding does two things. It enables or disables whether rounded node geometry is created during layout. But also after the layout is generated it controls whether the per node layout getter function returns rounded or unrounded geometry:

pub fn layout(&self, node: NodeId) -> TaffyResult<&Layout> {
        if self.config.use_rounding {
            Ok(&self.nodes[node.into()].final_layout)
        } else {
            Ok(&self.nodes[node.into()].unrounded_layout)
        }
    }

Hello ickshonpe 😀Thank you for the detailed breakdown! That explains why the API behaves that way, and it makes complete sense why we need to explicitly restore the rounding state after getting the unrounded size.

[ickshonpe]

I'm really confused now, does this actually introduce the bug that it's meant to fix? This regression test passes on main but fails with this PR:

    #[test]
    fn rounding_test() {
        let mut app = setup_ui_test_app();

        let root = app
            .world_mut()
            .spawn(Node {
                width: px(100.),
                height: px(100.),
                ..default()
            })
            .with_child((
                Node::default(),
                LayoutConfig {
                    use_rounding: false,
                },
            ))
            .id();

        app.update();

        assert_eq!(
            app.world().get::<ComputedNode>(root).unwrap().size(),
            Vec2::splat(100.)
        );

        app.world_mut().get_mut::<Node>(root).unwrap().width = px(200.);
        app.update();

        assert_eq!(
            app.world().get::<ComputedNode>(root).unwrap().size(),
            Vec2::new(200., 100.)
        );
    }

[jorgeandrecastro]

I'm really confused now, does this actually introduce the bug that it's meant to fix? This regression test passes on main but fails with this PR:

    #[test]
    fn rounding_test() {
        let mut app = setup_ui_test_app();

        let root = app
            .world_mut()
            .spawn(Node {
                width: px(100.),
                height: px(100.),
                ..default()
            })
            .with_child((
                Node::default(),
                LayoutConfig {
                    use_rounding: false,
                },
            ))
            .id();

        app.update();

        assert_eq!(
            app.world().get::<ComputedNode>(root).unwrap().size(),
            Vec2::splat(100.)
        );

        app.world_mut().get_mut::<Node>(root).unwrap().width = px(200.);
        app.update();

        assert_eq!(
            app.world().get::<ComputedNode>(root).unwrap().size(),
            Vec2::new(200., 100.)
        );
    }

Ah, good catch! 😅 You're completely right, looking back at the diff I missed the actual size fetching and state restoration part. Thanks for writing a regression test for this I'll properly fix it tomorrow! 🤵

pub fn get_layout(
       &mut self,
       entity: Entity,
       use_rounding: bool,
   ) -> Result<(taffy::Layout, Vec2), UiSurfaceError> {
       let Some(taffy_node) = self.entity_to_taffy.get(&entity) else {
           return Err(UiSurfaceError::NoAssociatedTaffyNode);
       };

       if use_rounding {
           self.taffy.enable_rounding();
       } else {
           self.taffy.disable_rounding();
       }

       let layout = self.taffy.layout(taffy_node.id).cloned();

       // Temporarily disable rounding to retrieve the unrounded layout size
       self.taffy.disable_rounding();
       let taffy_size = self.taffy.layout(taffy_node.id).unwrap().size;
       let unrounded_size = Vec2::new(taffy_size.width, taffy_size.height);

       // Restore the initial rounding state instead of unconditionally enabling it
       if use_rounding {
           self.taffy.enable_rounding();
       }

       match layout {
           Ok(l) => Ok((l, unrounded_size)),
           Err(e) => Err(UiSurfaceError::TaffyError(e)),
       }
   }

With this version ig should work , i will fixit tomorrow morning 😀

[ickshonpe]

Right okay I remember everything now, it's not the fault of the author here. It's my terrible design compounded by the terrible Taffy API and my lazyness in not adding any tests 😅 .

There isn't a bug. We always generate a rounded final_layout. It is bad though. What I think needs to be changed:

1. We shouldn't be setting rounding at the end of the get_layout function at all. Instead, we should call enable_rounding before before we call Taffy's compute_layout_with_measure in the UiSurface::compute_layout function.
2. Add some regression tests.,
3. Add some comments to get_layout and compute_layout explaining enable/disable_rounding's dual purpose.

[jorgeandrecastro]

Right okay I remember everything now, it's not the fault of the author here. It's my terrible design compounded by the terrible Taffy API and my lazyness in not adding any tests 😅 .

There isn't a bug. We always generate a rounded final_layout. It is bad though. What I think needs to be changed:

1. We shouldn't be setting rounding at the end of the get_layout function at all. Instead, we should call enable_rounding before before we call Taffy's compute_layout_with_measure in the UiSurface::compute_layout function.
2. Add some regression tests.,
3. Add some comments to get_layout and compute_layout explaining enable/disable_rounding's dual purpose.

Since you mentioned wanting to handle this further upstream in compute_layout, let me know how you'd like to proceed. Should I adjust my PR to follow your suggested approach, or are you planning to take care of the refactoring from your side? I'm happy to adapt or wait for your changes!.
It wasn't a "bug " ,cause its work's but is a Logic issue cause de never consider user choice 🤵

[alice-i-cecile]

@ickshonpe's plan should be followed here, but please let us know if you don't fully understand it, and we'll have someone else take this over to make sure it doesn't get dropped <3

[jorgeandrecastro]

plan should be followed here, but please let us know if you don't fully understand it, and we'll have someone else take this over to make sure it doesn't get dropped <3

Got it completely :) I've got it handled. I followed @ickshonpe's plan, and the regression test is passing successfully. Everything is formatted, pushed, and ready to go!, i hope

[ickshonpe]

Looks good.

[jorgeandrecastro]

Looks good.

I tester the rétrocompatible test its green and the other ones , so Hope its helps 😀