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

Turn the Visibility struct into an enum #6271

Closed
wants to merge 7 commits into from

Conversation

ickk
Copy link
Member

@ickk ickk commented Oct 16, 2022

Objective

The current implementation of the Visibility struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching.

Solution

Make Visibility into an enum.


Changelog

Changed

Visibility is now an enum

Migration Guide

  • Code that checks the visibility.is_visible field should now call it as a method visibility.is_inherited(), use ==, or refactor to use a match.
  • Code that used to set or unset the visibility.is_visible field should now directly set the value: *visibility = Visibility::Inherited, or use the .toggle() or .set(bool) methods.
  • Any usages of the associated constants Visibility::VISIBLE or Visibility::INVISIBLE should now use the enum variants, Visibility::Inherited or Visibility::Hidden.
  • ComputedVisibility::INVISIBLE and SpatialBundle::VISIBLE_IDENTITY have been renamed to ComputedVisibility::HIDDEN and SpatialBundle::INHERITED_IDENTITY respectively.

@ickk
Copy link
Member Author

ickk commented Oct 16, 2022

Since the variants are self descriptive, we could pub use them like std does with Option so that users can avoid the Visibility:: prefix.

@nicopap
Copy link
Contributor

nicopap commented Oct 16, 2022

If we are open to a major breaking change, may I suggest a change that improves one of the issues I have with Visibility which is the stuttering. ie: *visibility = Visibility::Invisible (repeats "visible" three times)

What about:

enum Visibility {
  Show,
  Hide
}

Previous way of doing it was visibility.is_visible = false

Also I think here a togglemethod is much needed for an enum-based API, since otherwise it would make it super annoying to swap the value (a bad good idea would be to implement std::ops::Not) (maybe two methods: one that returns a new value and one that mutably updates)

Edit: Adding the Visibility variants to the prelude is just equal part "good" and "bad" for me to be OK with it.

@nicopap nicopap added A-Rendering Drawing game state to the screen M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR labels Oct 16, 2022
@ickk
Copy link
Member Author

ickk commented Oct 16, 2022

Also I think here a toggle method is much needed for an enum-based API

see #6268 :)

Edit: decided to just include a .toggle() method in this PR since I agree that it is much needed

@mockersf
Copy link
Member

I'm not sure I think the is_visible method is really useful here

@sullyj3
Copy link
Contributor

sullyj3 commented Oct 16, 2022

I'm happy to close #6268 in favour of this if there's a consensus that an Enum is a nicer api

@sullyj3
Copy link
Contributor

sullyj3 commented Oct 16, 2022

I agree that is_visible feels redundant:

if vis == Visibility::Show { // ... }
if vis.is_visible() { //... }

The method is slightly terser, which is nice. Unless the variant was exported directly from the prelude:

if vis == Show { //... }

It would also depend on the variant names, eg in

if vis == Visibility::Visible { // ... }
if vis.is_visible() { //... }

the method seems much nicer.

Option has .is_none(), and that seems to be the idiomatic way to do things, despite the fact that you could just check == None there also.
Hmm, I guess to use == with Option<T>, T must be PartialEq, so .is_some() improves ergonomics in the case where it's not? Could that have been the rationale? We don't have that problem here.

I think I'm in favour of the method if the variants aren't exported unqualified, just because it's fewer characters.

@ickk
Copy link
Member Author

ickk commented Oct 17, 2022

I am more than happy to add an implementation for Eq, but I think the is_visible method should stay to provide a more consistent API with ComputedVisibility.

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'm in favor of using an enum here. Bikeshedding, I have a pretty strong preference for Visibility::Show and Visibility::Hide. Agree on adding toggle. is_visible can stay for consistency.

@mockersf
Copy link
Member

Could you impl the https://doc.rust-lang.org/std/ops/trait.Not.html trait and use it?

accept suggestion to use `matches!`
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Not sure I agree with this decision. While this improves simple assignments and serialized formats, this does make it notably harder to assign visibility state with even simple boolean expressions, instead requiring match/if expressions, which can be a lot less ergonomic.

crates/bevy_render/src/view/visibility/mod.rs Show resolved Hide resolved
@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 Oct 19, 2022
@sullyj3
Copy link
Contributor

sullyj3 commented Oct 19, 2022

this does make it notably harder to assign visibility state with even simple boolean expressions, instead requiring match/if expressions, which can be a lot less ergonomic.

What if there was a Visibility::from_bool()? Or I guess a From<bool> impl?

@ickk
Copy link
Member Author

ickk commented Oct 19, 2022

I agree could be more terse when modifying an existing instance using the result of a boolean expression.

I wrote out all the scenarios so I could evaluate using From/Into, and I did not really love it.

We could discuss adding something like a set_when method or something maybe?

visibility.set_when(a && b || !c);

@alice-i-cecile
Copy link
Member

Why not just visibility.set(visibility.set_when(a && b || !c)); where the set method takes a bool?

add a test ensuring that `Visibility` is one byte and that null pointer optimisation is doing its thing
@ickk ickk closed this Oct 20, 2022
bors bot pushed a commit that referenced this pull request Dec 25, 2022
Consolidation of all the feedback about #6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>
bors bot pushed a commit that referenced this pull request Dec 25, 2022
Consolidation of all the feedback about #6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>
bors bot pushed a commit that referenced this pull request Dec 25, 2022
Consolidation of all the feedback about #6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
Consolidation of all the feedback about bevyengine#6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
Consolidation of all the feedback about bevyengine#6271 as well as the addition of an "unconditionally visible" mode.

# Objective

The current implementation of the `Visibility` struct simply wraps a boolean.. which seems like an odd pattern when rust has such nice enums that allow for more expression using pattern-matching. 

Additionally as it stands Bevy only has two settings for visibility of an entity: 
- "unconditionally hidden" `Visibility { is_visible: false }`, 
- "inherit visibility from parent" `Visibility { is_visible: true }`
   where a root level entity set to "inherit" is visible. 

Note that given the behaviour, the current naming of the inner field is a little deceptive or unclear.

Using an enum for `Visibility` opens the door for adding an extra behaviour mode. This PR adds a new "unconditionally visible" mode, which causes an entity to be visible even if its Parent entity is hidden. There should not really be any performance cost to the addition of this new mode.

--
The recently added `toggle` method is removed in this PR, as its semantics could be confusing with 3 variants.

## Solution

Change the Visibility component into
```rust
enum Visibility {
  Hidden,    // unconditionally hidden
  Visible,   // unconditionally visible
  Inherited, // inherit visibility from parent
}
```

---

## Changelog

### Changed

`Visibility` is now an enum

## Migration Guide

- evaluation of the `visibility.is_visible` field should now check for `visibility == Visibility::Inherited`.
- setting the `visibility.is_visible` field should now directly set the value: `*visibility = Visibility::Inherited`.
- usage of `Visibility::VISIBLE` or `Visibility::INVISIBLE` should now use `Visibility::Inherited` or `Visibility::Hidden` respectively.
- `ComputedVisibility::INVISIBLE` and `SpatialBundle::VISIBLE_IDENTITY` have been renamed to `ComputedVisibility::HIDDEN` and `SpatialBundle::INHERITED_IDENTITY` respectively.






Co-authored-by: Carter Anderson <mcanders1@gmail.com>
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 M-Needs-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 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.

7 participants