Skip to content

Conversation

TotalKrill
Copy link
Contributor

Objective

To reduce friction when writing Ui code by letting the code follow the left-to-right mindset.

Mental model: "The width should be 20 pixels" , as in "affect this, with value,of unit"

instead of:
ui_rect.width = Val::Px(20.0);
let us go:
ui_rect.width = 20.px();

Solution

Implement a trait in geometry and implement for the common types

Testing

Tested this by modifying the text_debug example to use this instead and it worked.


@TotalKrill TotalKrill changed the title add AsVal trait that helps with constructing values add AsVal trait that helps with constructing Val Oct 10, 2024
@alice-i-cecile alice-i-cecile requested a review from cart October 10, 2024 15:51
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Contentious There are nontrivial implications that should be thought through labels Oct 10, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I like the syntax, and the implementation is clean. The use a macro is justified here IMO: there's a ton of boilerplate and the code is super simple.

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 10, 2024
@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 10, 2024
@alice-i-cecile
Copy link
Member

Bumping to the 0.16 milestone since this is implementable externally and it's a new feature :)

@alice-i-cecile alice-i-cecile 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 Oct 10, 2024
@alice-i-cecile
Copy link
Member

If we're going to be doing this, we should export the trait in the prelude TBH.

@TotalKrill
Copy link
Contributor Author

TotalKrill commented Oct 10, 2024 via email

@cart
Copy link
Member

cart commented Oct 10, 2024

As a heads up, within the context of BSN I was planning on doing: px(10.0). That will notably play nicer with function reflection in the context of scene assets, as this type of trait-function-reflection requires manual special casing to resolve. Idk if we should have two very similar ways to do this / better to have one way that works in every situation.

@alice-i-cecile
Copy link
Member

I'm fine with either solution. This plays a bit nicer with autocomplete. We should merge one or the other for 0.16 though :)

@cart
Copy link
Member

cart commented Oct 10, 2024

This plays a bit nicer with autocomplete

The downside here (if this is in the prelude) is that it adds .px() to autocomplete for every float in every context.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through labels Oct 10, 2024
@alice-i-cecile alice-i-cecile added the S-Needs-SME Decision or review from an SME is required label Oct 10, 2024
@ickshonpe
Copy link
Contributor

ickshonpe commented Oct 15, 2024

It got shot down the last time I suggested it, but I still think the lowest friction option for users is to change all the Val function arguments to Into<Val> and impl From f32 and i32 for Val mapping them to Val::Px.

@cart
Copy link
Member

cart commented Oct 15, 2024

I'm closing this in favor of px(10.)-style helper functions (or alternatively, exporting Val::Px as Px() in the prelude)

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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-SME Decision or review from an SME is required X-Controversial There is active debate or serious implications around merging this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants