Skip to content

Conversation

@ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Nov 23, 2025

Objective

In UI and world coordinates, “top” and “bottom” mean opposite things, so these fields are ambiguous. Anyone using BorderRect must decide themselves how to intepret the vertical inset values.

Fixes #21913

For more in detail motivation, see the replies to issue #21913

Solution

Replace the directional BorderRect fields (left, right, top, and bottom) with min_inset and max_inset Vec2 fields.

Using min_inset and max_inset removes the need to interpret top or bottom relative to the coordinate system, so the same logic will work consistently in both UI and 2D.

@ickshonpe ickshonpe added D-Trivial Nice and easy! A great choice to get started with Bevy A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 25, 2025
@ickshonpe ickshonpe added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 26, 2025
Copy link
Contributor

@kfc35 kfc35 left a comment

Choose a reason for hiding this comment

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

Thank you for this!

right: horizontal,
top: vertical,
bottom: vertical,
min_inset: insets,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency with the other constructors, you can assign these insets to separate Vec2::new’s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean, this?

    pub const fn axes(horizontal: f32, vertical: f32) -> Self {
        Self {
            min_inset: Vec2::new(horizontal, vertical),
            max_inset: Vec2::new(horizontal, vertical),
        }
    }

I prefer the the single Vec2 constructor version I think as less error prone, though the difference is very marginal.

Copy link
Contributor Author

@ickshonpe ickshonpe Nov 28, 2025

Choose a reason for hiding this comment

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

I also considered swapping axes parameters to a Vec2. I think it might be more ergonomic, but a vector implies direction. The inputs here are directionless, so the semantics with separate f32's feel better to me.

Copy link

@Hilpogar Hilpogar left a comment

Choose a reason for hiding this comment

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

From what I understand, the min_inset is supposed to be the top_left corner in the UI coordinates and the bottom_left corner in the 2D World coordinate. But in the code, you consistently used the top_left corner as the min_inset for both the sprite and the UI. Is it an error or am I missing something ?

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Dec 10, 2025

From what I understand, the min_inset is supposed to be the top_left corner in the UI coordinates and the bottom_left corner in the 2D World coordinate. But in the code, you consistently used the top_left corner as the min_inset for both the sprite and the UI. Is it an error or am I missing something ?

Yep the compute_slice functions are written in a weird way, but this is correct. Because it was so confusing, I modified the sprite_slice example to use a non-homogeneous border to make sure:

a_slice slicer

It's not worth cleaning up compute_slice though, instead the 2d sprite slicer needs to be rewritten to use a shader like the UI version does. You can see visual artifacts in the screenshot above due to floating point errors leaving gaps between the slices too, which wouldn't be present with a shader based solution. Plus the performance is abysmal, it's drawing about 50,000 sprites just to display those four bordered rects.

@ickshonpe ickshonpe requested a review from Hilpogar December 10, 2025 10:01
@pablo-lua pablo-lua 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 Dec 16, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 16, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 16, 2025
Copy link

@Hilpogar Hilpogar left a comment

Choose a reason for hiding this comment

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

Thank you for the visual checking !

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 16, 2025
Merged via the queue into bevyengine:main with commit 880149a Dec 16, 2025
36 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 C-Code-Quality A section of code that is hard to understand or change D-Trivial Nice and easy! A great choice to get started with Bevy M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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

Development

Successfully merging this pull request may close these issues.

BorderRect is ambiguous

5 participants