Rename BorderRect to Insets and move to bevy_math

[rudderbucky]

Objective

Fixes #15440.

BorderRect is used by more than bevy_sprite at this point and adds value not present in the standard Rect as it does not use Vec2. It should therefore be moved to bevy_math so other similar use cases can use the struct.

The name of BorderRect may also be subject to debate as it's not necessarily used only for borders (or anything UI-related for that matter).

Solution

• Move BorderRect from bevy_sprite to bevy_math
• Rename BorderRect to Insets (please suggest any names you think may be better)

Testing

Ran cargo run -p ci on macOS, it looks good. Let me know if I should add unit tests.

Migration Guide

Any uses of bevy_sprite::BorderRect need to instead be pointed to bevy_math::Insets.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[UkoeHB]

Seems fine, the name is clear enough in context.

[ickshonpe]

Looks good, don't have any problems with it.

[janhohenheim]

Looks good overall, good idea :)
This now loses documentation that the units in question are pixels for UI. It makes sense to remove that doc from Insets, as that is a pure math type now and does not know about UI. Could we instead document it on the various border fields?

Also: would it be alright to rename Insets to Inset?

[ickshonpe]

Also: would it be alright to rename Insets to Inset?

Yeah prefer Inset.

[Jondolf]

So personally, I don't really like moving things like this into bevy_math. bevy_math is intended to be a sort of generally useful "math for gamedev" crate, and not have too much Bevy-specific functionality. Imo the Rect types already shouldn't be in bevy_math, and this takes it a step further since BorderRect/Inset is a very visual type with the only difference from the existing rectangle types being semantics and that it stores four numbers instead of two vectors. Why would bevy_math have random UI or texture slicing types?

Ignoring "bevy_math purity", this removes the contextual semantics of BorderRect in favor of a general "inset", which is much more vague. In the UI context, this is heavily confusing, since UI borders are typically not insets, and the CSS property inset is a completely different thing from borders. And this again is different from what it represents for texture slicing in bevy_sprite.

What I would do instead is:

• Just use Rect for the internal computed border in bevy_ui. The user-facing APIs already use UiRect anyway, the Rect type (or currently BorderRect) simply stores the computed logical border thickness in pixels. As far as I can see, there's no reason it can't use a Vec2 instead of the four separate values.
• Keep BorderRect in bevy_sprite, and maybe rename it to Inset there, or even something like SlicingInset since it only seems to be used for TextureSlicer, defining edges that slice textures into a grid of 9 parts, i.e. 9-slicing.
  • Or if we want to reuse existing types, why not just use Rect there too? TextureSlice already uses it for texture_rect, and our paddings and margins use rect types (even if it is a bit confusing). We could just remove BorderRect altogether unless we want the documented semantics. We already have too many rectangle types.

This would decouple bevy_ui from bevy_sprite for this specific type, while keeping semantics accurate (or at least avoid making them even more confusing). I don't see value in adding yet another rectangle type to bevy_math with a slightly different representation and confusing semantics in comparison to the existing types. If we want separate types for clearer semantics, the types should live close to where they are used, and be tailored for their specific use case with appropriate documentation.

[BenjaminBrienen]

I don't think this makes sense in bevy_math for most of the reasons Jondolf mentioned already.

[Jondolf]

To be clear, I do think it may be good to replace our use of UiRect for paddings, margins, and borders, since it's not really a rectangle in itself, at least in the way you'd intuitively think about it. Flutter uses EdgeInsets for padding and margin, and has a separate Border type for borders, storing the thickness and color of each BorderSide. I think the term "inset" is alright (but not ideal, though I can't think of a better name) for things like padding, but for borders it's confusing. And whatever the types are called, if they are strongly related to UI like inset is, they shouldn't be in bevy_math.

[janhohenheim]

@Jondolf I'm not a UI person, but your arguments sound very convincing to me. I retract my statement about this looking alright.

[rudderbucky]

@Jondolf agree it's not really a rectangle in itself, which is the main value I see in BorderRect... I see value in it beyond just using Rect because you can explicitly define that you want padding in one of the four directions, rather than having to redundantly specify the other direction as 0 when using Vec2s (or needlessly getting into how to use Vec2 IMO) Would it make sense to keep it but in bevy_ui instead?

re: padding vs borders, I also think it's a better name for padding, but I think it would make enough sense in context for borders too. Maybe we could add a separate class for borders and make it less confusing that way.

@janhohenheim re: renaming to Inset instead of Insets, I chose the plural form because (my interpretation) the struct really represents four insets in the four directions rather than just one, I felt that would make the purpose of the struct clearer. LMK what you think.

[rudderbucky]

@Jondolf friendly ping on feedback for this

[clarfonthey]

So, thinking about this, kind of see this type as an asymmetric version of Rect; effectively, if Rect + Rect adds the min and max, then Rect + Inset, will add the max, but subtract the min.

I do think that this representation would be valuable in bevy_math, but I kind of do think that it should be represented with two vectors, rather than four scalars, going more on this representation. But having a different version could be valuable.

I am fine with Inset/Insets as a name though.

[Jondolf]

I see value in it beyond just using Rect because you can explicitly define that you want padding in one of the four directions, rather than having to redundantly specify the other direction as 0 when using Vec2s

Yeah, that's definitely a fair point. I agree Rect isn't ideal for this. However, the user-facing API in bevy_ui uses UiRect everywhere. BorderRect is only used for internal node layout data, which is private, and could pretty easily be replaced with Rect without regressing user ergonomics.

Would it make sense to keep it but in bevy_ui instead?

As I mentioned, it's not really used in bevy_ui, while it is used in bevy_sprite for 9-slicing. Imo it should keep living in bevy_sprite, under whatever name makes the most sense there. Nothing in Bevy uses BorderRect except 9-slicing and those internal UI node border properties that could be changed to use another type.

[rudderbucky]

@Jondolf IMO I don't think replacing BorderRect with Rect is an option as it would introduce ambiguity in which Vec2 index represents a specific direction. This would still be a problem if it was internal-only, IMO.

We could try UiRect, but since it uses Val, I'm not sure how we would define inner_radius under ui_node.rs which assumes border is made up of f32s. My understanding of UiRect is that it's a class meant to abstract away size calculations w.r.t. by parents/viewports. If so, I would also prefer against replacing the border field in Node with UiRect as that field is explicitly post-calculation.