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

Pixel gap between adjacent UI grid cells #9754

Closed
yrns opened this issue Sep 10, 2023 · 6 comments · Fixed by #9784
Closed

Pixel gap between adjacent UI grid cells #9754

yrns opened this issue Sep 10, 2023 · 6 comments · Fixed by #9784
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior

Comments

@yrns
Copy link
Contributor

yrns commented Sep 10, 2023

Bevy version

11.2

Relevant system information

INFO winit::platform_impl::platform::x11::window: Guessed window scale factor: 1.1666666666666667
INFO bevy_render::renderer: AdapterInfo { name: "NVIDIA GeForce GTX 970", vendor: 4318, device: 5058, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "535.98", backend: Vulkan }

What you did

use bevy::prelude::*;

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

fn setup(mut commands: Commands) {
    commands.spawn(Camera2dBundle::default());

    commands
        .spawn(NodeBundle {
            style: Style {
                display: Display::Grid,
                grid_template_columns: RepeatedGridTrack::min_content(2),
                margin: UiRect::all(Val::Px(4.0)),
                ..Default::default()
            },
            background_color: BackgroundColor(Color::PINK),
            ..Default::default()
        })
        .with_children(|commands| {
            for color in [Color::GRAY, Color::DARK_GRAY] {
                commands.spawn(NodeBundle {
                    style: Style {
                        display: Display::Grid,
                        width: Val::Px(160.),
                        height: Val::Px(160.),
                        ..Default::default()
                    },
                    background_color: BackgroundColor(color),
                    ..Default::default()
                });
            }
        });
}

What went wrong

The vertical pink line seems to be related to the margin, as it doesn't appear with a zero margin. But varying values of width and height for the cells also make the gap disappear. Maybe related to #7712, although not a regression (I tested the sample on 11.2 and it works).

grid_gap

@yrns yrns added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Sep 10, 2023
@rparrett rparrett added A-UI Graphical user interfaces, styles, layouts, and widgets and removed S-Needs-Triage This issue needs to be labelled labels Sep 11, 2023
@rparrett
Copy link
Contributor

I can reproduce by using the same scale factor. Seems like probably some rounding bug here or in Taffy.

For the convenience of anyone else taking a look:

        .add_plugins(DefaultPlugins.set(WindowPlugin {
            primary_window: Some(Window {
                resolution: WindowResolution::default().with_scale_factor_override(7. / 6.),
                ..default()
            }),
            ..default()
        }))

@ickshonpe
Copy link
Contributor

ickshonpe commented Sep 11, 2023

Yep, replicates on main.

The example needs to be recreated in Taffy, yrns want to carry on with this?

@yrns
Copy link
Contributor Author

yrns commented Sep 11, 2023

Yep, replicates on main.

The example needs to be recreated in Taffy, yrns want to carry on with this?

I can try, sure.

@nicoburns
Copy link
Contributor

I thought this might be a bug in Taffy's rounding but I was unable to reproduce the issue. The following test (which is intended to be the original layout above, with the scale factor pre-applied to the styles):

<div id="test-root">
  <div style="display: grid; grid-template-columns: min-content min-content;margin: 4.66666666667px; background: pink">
   <div style="width: 186.666666667px; height: 186.666666667px"></div>
   <div style="width: 186.666666667px; height: 186.666666667px"></div>
  </div>
</div>

Results in the following gapless layout:

TREE
└──  FLEX [x: 0    y: 0    width: 383  height: 196 ] (NodeId(4294967300))
    └──  GRID [x: 5    y: 5    width: 373  height: 186 ] (NodeId(4294967299))
        ├──  LEAF [x: 0    y: 0    width: 186  height: 186 ] (NodeId(4294967297))
        └──  LEAF [x: 187  y: 0    width: 187  height: 186 ] (NodeId(4294967298))

However, I have remembered that Bevy isn't currently using Taffy's rounding. So I don't think a bug in Taffy's rounding would even affect Bevy.


We could try switching Bevy back to using Taffy's rounding now that the bugs with that have been fixed (would need to bump minimum Taffy version to 0.3.13). But looking at the Bevy code, it looks like we're currently scaling values after rounding:

Screenshot 2023-09-12 at 00 15 29

I suspect such code is liable to cause issues like this even is rounding is performed correctly. I believe the pixel-grid snapping really needs to be the last step in the process if it is the effectively eliminate gaps.

@yrns
Copy link
Contributor Author

yrns commented Sep 11, 2023

FWIW, I could not reproduce in Taffy either. This is what I get in Bevy (11.2, taffy 0.3.10) for comparison:

TREE
└──  FLEX [x: 0    y: 0    width: 1252 height: 1361] (1v1)
    └──  GRID [x: 4.6666665 y: 4.6666665 width: 373.33334 height: 1351.6666] (4v1)
        ├──  LEAF [x: 0    y: 0    width: 186.66667 height: 186.66667] (2v1)
        └──  LEAF [x: 186.66667 y: 0    width: 186.66667 height: 186.66667] (3v1)

@ickshonpe
Copy link
Contributor

ickshonpe commented Sep 12, 2023

I suspect such code is liable to cause issues like this even is rounding is performed correctly. I believe the pixel-grid snapping really needs to be the last step in the process if it is the effectively eliminate gaps.

This makes sense to me. I made the change and it seems to work, branch here: https://github.com/ickshonpe/bevy/tree/round-after-scaling

So, from what I can understand, for rounding to work correctly all the time either:

  1. Taffy needs to support scale factor so it can apply the scaling before rounding
  2. We pass already scaled logical coordinates to Taffy, which it then rounds
  3. Taffy doesn't perform any rounding, we scale then round the values returned from UiSurface::get_layout

github-merge-queue bot pushed a commit that referenced this issue Sep 18, 2023
# Objective

Fixes #9754

## Solution

Don't round UI coordinates until they've been multiplied by the inverse
scale factor.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
# Objective

Fixes bevyengine#9754

## Solution

Don't round UI coordinates until they've been multiplied by the inverse
scale factor.
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants