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.

Sign up for GitHub

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

Jump to bottom

Make GridPlacement's fields non-zero and add accessor functions. #9486

Merged
merged 9 commits into from
Aug 28, 2023
Merged

Make GridPlacement's fields non-zero and add accessor functions. #9486

merged 9 commits into from
Aug 28, 2023

Conversation

ickshonpe
Copy link
Contributor

@ickshonpe ickshonpe commented Aug 18, 2023 •
edited
Loading

Objective

  • There is no way to read the fields of GridPlacement once set.
  • Values of 0 for GridPlacement's fields are invalid but can be set.
  • A non-zero representation would be half the size.

fixes #9474

Solution

  • Add get_start, get_end and get_span accessor methods.
  • ChangeGridPlacement's constructor functions to panic on arguments of zero.
  • Use non-zero types instead of primitives for GridPlacement's fields.

Changelog

bevy_ui::ui_node::GridPlacement:

  • Field types have been changed to Option<NonZeroI16> and Option<NonZeroU16>. This is because zero values are not valid for GridPlacement. Previously, Taffy interpreted these as auto variants.
  • Constructor functions for GridPlacement panic on arguments of 0.
  • Added accessor functions: get_start, get_end, and get_span. These return the inner primitive value (if present) of the respective fields.

Migration Guide

GridPlacement's constructor functions no longer accept values of 0. Given any argument of 0 they will panic with a GridPlacementError.

@ickshonpe
bevy_ui::ui_node::GridPlacement:
ac8ad76 
* Field types have been changed to `Option<NonZeroI16>` and `Option<NonZeroU16>`. This is because zero values are not valid for `GridPlacement`. Previously, Taffy interpreted these as auto variants.
* Constructor functions for `GridPlacement` that accept numeric values now return `Result<Self, GridPlacementError>`. An error of type `GridPlacementError` is returned on arguments equal to zero.
* Added accessor functions: `get_start`, `get_end`, and `get_span`. These return the inner primitive value (if present) of the respective fields.
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@ickshonpe
Returning a result from the constructor functions feels like overengi…
5bad4e1 
…neering. Switched the functions back so they return `Self unless given a zero valued argument in which they will panic with a `GridPlacementError`.
@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 19, 2023
alice-i-cecile
alice-i-cecile requested changes Aug 19, 2023
View reviewed changes
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 this approach, but it needs some more polish.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@alice-i-cecile
Copy link
Member

Once there's docs for GridPlacementError this LGTM. Tests never hurt, but I'll leave that to you.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@alice-i-cecile
Copy link
Member

@viridia can I get your review on this PR? If this looks good to you and meets your needs, I'll be able to merge it with your approval :) See the relevant section of CONTRIBUTING.md for more details on the community approval model we use.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

1 similar comment
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Aug 19, 2023 •
edited
Loading

Once there's docs for GridPlacementError this LGTM. Tests never hurt, but I'll leave that to you.

Added a few trivial tests but there isn't much here to check; the logic is all in layout's convert function and Taffy.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

2 similar comments
@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

@github-actions
Copy link
Contributor

Example alien_cake_addict failed to run, please try running it locally and check the result.

viridia
viridia approved these changes Aug 19, 2023
View reviewed changes
Copy link
Contributor

@viridia viridia left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 19, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit a2bd93a Aug 28, 2023
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@ickshonpe
Make GridPlacement's fields non-zero and add accessor functions. (b…
edc17cd 
…evyengine#9486)

# Objective

* There is no way to read the fields of `GridPlacement` once set. 
* Values of `0` for `GridPlacement`'s fields are invalid but can be set.
* A non-zero representation would be half the size.

fixes bevyengine#9474

## Solution
* Add `get_start`, `get_end` and `get_span` accessor methods.
* Change`GridPlacement`'s constructor functions to panic on arguments of
zero.
* Use non-zero types instead of primitives for `GridPlacement`'s fields.

---

## Changelog

`bevy_ui::ui_node::GridPlacement`:
* Field types have been changed to `Option<NonZeroI16>` and
`Option<NonZeroU16>`. This is because zero values are not valid for
`GridPlacement`. Previously, Taffy interpreted these as auto variants.
* Constructor functions for `GridPlacement` panic on arguments of `0`.
* Added accessor functions: `get_start`, `get_end`, and `get_span`.
These return the inner primitive value (if present) of the respective
fields.

## Migration Guide
`GridPlacement`'s constructor functions no longer accept values of `0`.
Given any argument of `0` they will panic with a `GridPlacementError`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viridia viridia viridia approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

Assignees
No one assigned
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GridPlacement fields cannot be accessed out of crate
3 participants
@ickshonpe @alice-i-cecile @viridia