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

UI Node's transform is sometimes incorrectly calculated #13517

Closed
porkbrain opened this issue May 26, 2024 · 17 comments · Fixed by #13555
Closed

UI Node's transform is sometimes incorrectly calculated #13517

porkbrain opened this issue May 26, 2024 · 17 comments · Fixed by #13555
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior S-Needs-Investigation This issue requires detective work to figure out what's going wrong

Comments

@porkbrain
Copy link
Contributor

Context

  • Bevy 0.13.2
  • Ubuntu 22.04.4
  • Resolution 3840x1600
2024-05-26T11:06:44.685619Z  INFO log: Guessed window scale factor: 1    
2024-05-26T11:06:44.720176Z  WARN log: InstanceFlags::VALIDATION requested, but unable to find layer: VK_LAYER_KHRONOS_validation    
2024-05-26T11:06:44.728709Z  INFO log: Debug utils not enabled: debug_utils_user_data not passed to Instance::from_raw    
2024-05-26T11:06:44.740176Z  INFO log: Using X11 platform    
2024-05-26T11:06:44.811299Z  WARN log: Detected skylake derivative running on mesa i915. Clears to srgb textures will use manual shader clears.    
2024-05-26T11:06:44.811326Z  INFO log: Adapter Vulkan AdapterInfo { name: "NVIDIA GeForce RTX 3080 Laptop GPU", vendor: 4318, device: 9372, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "545.29.06", backend: Vulkan }    
2024-05-26T11:06:44.811348Z  INFO bevy_render::renderer: AdapterInfo { name: "NVIDIA GeForce RTX 3080 Laptop GPU", vendor: 4318, device: 9372, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "545.29.06", backend: Vulkan }

What you did

I spawned a UI node that acts as a root for its child nodes. It's attached to the bottom of the screen and full screen width.

cmd
            .spawn((
                Name::new("Portrait dialog root"),
                DialogUiRoot,
                TargetCamera(camera),
                NodeBundle {
                    // centers the content
                    style: Style {
                        width: Val::Vw(100.0),
                        bottom: Val::Px(0.0),
                        position_type: PositionType::Absolute,
                        flex_direction: FlexDirection::RowReverse,

                        ..default()
                    },
                    ..default()
                },
            ))

What went wrong

When I spawn this node for the first time it has about one in five chance that it would have an incorrect transform x calculated (making it invisible.)

Here's what the transform value is when it loads incorrectly:
Screenshot from 2024-05-26 13-08-20

Here's what the value is when it loads correctly.
Also, I can fix an incorrect spawn by changing the position from absolute to relative and back to absolute again in the egui, after which the value is corrected:
Screenshot from 2024-05-26 13-08-48

@porkbrain porkbrain added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels May 26, 2024
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Investigation This issue requires detective work to figure out what's going wrong and removed S-Needs-Triage This issue needs to be labelled labels May 26, 2024
@alice-i-cecile
Copy link
Member

Sounds like a system ordering issue if it's occurring irregularly.

@porkbrain
Copy link
Contributor Author

porkbrain commented May 26, 2024

I thought perhaps the fact that I was not setting a height and relying on the content to expand that node might have been be the issue. Setting min height in the styles did not help though.

Also changing pretty much any Styles property triggers reevaluation and sets the Transform component to its correct value.

What's a good way for me to provide more information? I tried RUST_LOG=bevy_ui=trace but this yields not logs.

@alice-i-cecile
Copy link
Member

I'd really like a minimal reproducible example. Failing that, a reproducible example at all so we can test fixes would be super useful.

Normally I'd tell you to test this upstream in taffy, but this definitely doesn't seem like the issue. Testing it against main (after #10690) might be useful though.

@porkbrain
Copy link
Contributor Author

porkbrain commented May 26, 2024

I seperated out the relevant code to this repo. When I run this repo a few times, I encounter the problem.

  1. Run the code
  2. Press spacebar
  3. Close the project
  4. Repeat until one time the UI is spawned but not visible

@porkbrain
Copy link
Contributor Author

The culprit seems to be the WorldInspectorPlugin. If I don't add it, I am unable to reproduce the issue after many runs. With it, it's that 1/5 chance.

Now, it's unclear whether merely adding some extra systems increases the odds of this happening due to, as you pointed out, some specific ordering condition, or whether the plugin directly breaks something that bevy_ui calculations rely on.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 28, 2024

@porkbrain Looks like as in #13155 there is a problem in bevy_ui (or rather in taffy api), when there is only one dimension (width or height) is set with PositionType::Absolute. As workaround you can add height: Val::Px(384.0) in root Style instance, then behavour of your UI becomes much more stable. At least, I am unable to reproduce the issue with this change.

@bugsweeper
Copy link
Contributor

This workaround is not perfect. The issue is much more rare, but still exists even with it.
I think we should compare more info from root's data
Correct values
image
Uncorrect values
Screenshot_20240528_124314
I think that the most valuable is Node::calculated_size.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 28, 2024

I think

pub fn toggle_spawn(
    mut cmd: Commands,
    asset_server: Res<AssetServer>,

    camera: Query<Entity, With<DialogCamera>>,
    root: Query<Entity, With<DialogUiRoot>>,
    windows: Query<&Window, With<PrimaryWindow>>,
) {
    ...
    let width = if let Ok(window) = windows.get_single() {
        Val::Px(window.width())
    } else {
        Val::Vw(100.0)
    };

    let root = cmd
        .spawn((
            Name::new("Portrait dialog root"),
            DialogUiRoot,
            TargetCamera(camera),
            RenderLayers::layer(25),
            NodeBundle {
                // centers the content
                style: Style {
                    width,
                    bottom: Val::Px(0.0),
                    position_type: PositionType::Absolute,
                    flex_direction: FlexDirection::RowReverse,

                    ..default()
                },
                ..default()
            },
        ))
        .id();

would be better temporary workaround, it behaves well when toggling, but not when resizing

@nicoburns
Copy link
Contributor

nicoburns commented May 28, 2024

Agree that this is another instance of #13155

I thought perhaps the fact that I was not setting a height and relying on the content to expand that node might have been be the issue. Setting min height in the styles did not help though.

Hmm... this was also my understanding of the issue.

EDIT: Ok, I've found another issue that may have caused the min-width to be ignored. #13555 is up that should hopefully fix both issues.

@porkbrain
Copy link
Contributor Author

Thanks everyone for their support. Given that the linked issue is very active and the suggested temporary workaround, I will close this and pay attention to #13555. Once 0.14 is released, assuming the fix will be included, I will test this again and reopen in case I observe the behavior.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 29, 2024

@porkbrain I think you should reopen this issue, because taffy's ratio blindness is just a part of problem, the issue which we mentioned contains description of bug, that is stable and relates to images, but root node here is not image rather container! Every run we get same result there, but bug in this issue is floating. Second part (more important) of this problem is that some times node lives in zero-sized environment!
I tried figure out why calculated size of node contains zero and here is results of investigation:

  1. calculated_size gets zero from rounded_size, which gets zero from layout_size, which gets zero from layout, which gets zero from ui_surface, which takes this info from taffy. Why?
  2. Because earlier, before starting this recursion ui_surface feeds taffy layout with available_space, which is set by render_target_resolution, which was taken from camera.size. All of them some times are (0, 0). Why?
  3. Because camera.size was set by (0, 0) earlier from camera.physical_viewport_size(), which gets zeros from physical_target_size(), which gets zeros from camera.computed.target_info.physical_size. Why?
  4. Because camera.computed.target_info is set from new_computed_target_info, which is set from normalized_target under certain conditions. So why this mechanism sometimes does not work correctly?
  5. I didn't figured out yet, but I could try later. I assume that the sequence of launching the systems for spawning camera, attaching it to window and determining it's properties is important here. Maybe @Aceeri or @Weibye can correct me here?

Any way looks like taffy is not the source of problem here rather victim of circumstances

@Aceeri
Copy link
Member

Aceeri commented May 30, 2024

Some experimenting I did a while back is that the resolution sometimes gets set to 0, 0 when minimized and under other certain conditions depending on the platform. But I don't understand why that would stick rather than get updated here.

I would probably check if the window's physical resolution matches the camera's computed target info, my guess is there is a chain here where it misses an update about this.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 30, 2024

why that would stick rather than get updated here

Looks like camera fixes it's resolution later, but taffy still continues to store zeros in layout despite being given the correct data at step 2

when minimized and under other certain conditions depending on the platform

My reproducing flow is restarting app from reproduce instructions until problem occurs (determine unsuccessfull launch), then I can repeat problem again just by toggling nodes without app restart.

@bugsweeper
Copy link
Contributor

bugsweeper commented May 30, 2024

Update:

why that would stick rather than get updated here

The reason is that compute_camera_layout() updates resolution in root_nodes.implicit_viewport_node which containes node for viewport (this is correct, because available_space relates to viewport), but then update_uinode_geometry_recursive() updates geometry starting from user ui root node which is just a child of viewport node. So user ui root node doesn't update according to updated camera.size. @bardt, please tell us, shouldn't camera.root_nodes contain node of viewport instead of ui root node?

github-merge-queue bot pushed a commit that referenced this issue May 30, 2024
…zes (#13555)

# Objective

- Fixes #13155
- fixes #13517
- Supercedes #13381
- Requires DioxusLabs/taffy#661

## Solution

- Taffy has been updated to:
    - Apply size styles to absolutely positioned children
    - Pass the node's `Style` through to the measure function
- Bevy's image measure function has been updated to make use of this
style information

## Notes

- This is currently using a git version of Taffy. If this is tested as
fixing the issue then we can turn that into a Taffy 0.5 release (this
would be the only change between Taffy 0.4 and Taffy 0.5 so upgrading is
not expected to be an issue)
- This implementation may not be completely correct. I would have
preferred to extend Taffy's gentest infrastructure to handle images and
used that to nail down the correct behaviour. But I don't have time for
that atm so we'll have to iterate on this in future. This PR at least
puts that under Bevy's control.

## Testing

- I manually tested the game_menu_example (from
#13155)
- More testing is probably merited

---

## Changelog

No changelog should be required as it fixes a regression on `main` that
was not present in bevy 0.13. The changelog for "Taffy upgrade" may want
to be changed from 0.4 to 0.5 if this change gets merged.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: François Mockers <francois.mockers@vleue.com>
@bugsweeper
Copy link
Contributor

bugsweeper commented May 31, 2024

@alice-i-cecile I think we should reopen this issue again, I left additional info in previous comment about reason why this issue wants another PR.
My plan is to migrate reproducing example to 0.14.0-dev (main) and to be abble check future fix, because before now I debugged 0.13.2 version

@bugsweeper
Copy link
Contributor

bugsweeper commented May 31, 2024

@alice-i-cecile I did migrate example to bevy main (with #13555 fix) and example became much more stable (I ran ~30 times example to reproduce bug), but bug still persist. I will make fix PR according to syncing viewport and root ui node

@nicoburns
Copy link
Contributor

@bugsweeper Maybe open a new issue tracking the viewport updating issue

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-Needs-Investigation This issue requires detective work to figure out what's going wrong
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants