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

Support operations for render layers and fix equality comparisons #13310

Merged
merged 18 commits into from
Jul 1, 2024

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented May 10, 2024

Objective

Allow combining render layers with a more-ergonomic syntax than RenderLayers::from_iter(a.iter().chain(b.iter())).

Solution

Add the or operation (and corresponding const method) to allow computing the union of a set of render layers. While we're here, also added and and xor operations. Someone might find them useful

Testing

Added a simple unit test.

Comment on lines 120 to 126
pub const fn and(self, other: Self) -> Self {
let mask = self.0 & other.0;
Self(mask)
}

/// Returns all [layers](Layer) included in either instance of [`RenderLayers`].
pub const fn or(self, other: Self) -> Self {
Copy link
Member Author

Choose a reason for hiding this comment

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

I considered calling these intersection and union for clarity, but I wasn't sure what to call xor using that naming convention

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be the symmetric difference or disjunctive union. Pretty obscure so not sure it's worth switching. Note that Python does have a symmetric difference operation on sets. https://en.m.wikipedia.org/wiki/Symmetric_difference

Copy link
Member Author

Choose a reason for hiding this comment

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

It probably is worth switching, at least to avoid potential confusion with the and method. I imagine some people may think a.and(b) means "all the layers from a and b"

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.

LGTM other than the renaming.

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 13, 2024
@tychedelia
Copy link
Contributor

tychedelia commented May 13, 2024

if we can do #13317 would mean these can't be const, not sure how const-ness affects their value for you

@janhohenheim janhohenheim 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 Jun 25, 2024
@JoJoJet JoJoJet changed the title Support operations for render layers Support operations for render layers and fix equality comparisons Jun 30, 2024
@JoJoJet JoJoJet added the C-Bug An unexpected or incorrect behavior label Jun 30, 2024
Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Thanks! The bug you identify in without potentially should make this included in 0.14 imo cc: @alice-i-cecile.

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 30, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 1, 2024
Merged via the queue into bevyengine:main with commit 64bc811 Jul 1, 2024
27 checks passed
mockersf pushed a commit that referenced this pull request Jul 2, 2024
…3310)

# Objective

Allow combining render layers with a more-ergonomic syntax than
`RenderLayers::from_iter(a.iter().chain(b.iter()))`.

## Solution

Add the `or` operation (and corresponding `const` method) to allow
computing the union of a set of render layers. While we're here, also
added `and` and `xor` operations. Someone might find them useful

## Testing

Added a simple unit test.
zmbush pushed a commit to zmbush/bevy that referenced this pull request Jul 3, 2024
…vyengine#13310)

# Objective

Allow combining render layers with a more-ergonomic syntax than
`RenderLayers::from_iter(a.iter().chain(b.iter()))`.

## Solution

Add the `or` operation (and corresponding `const` method) to allow
computing the union of a set of render layers. While we're here, also
added `and` and `xor` operations. Someone might find them useful

## Testing

Added a simple unit test.
MrGVSV pushed a commit to MrGVSV/bevy that referenced this pull request Jul 5, 2024
…vyengine#13310)

# Objective

Allow combining render layers with a more-ergonomic syntax than
`RenderLayers::from_iter(a.iter().chain(b.iter()))`.

## Solution

Add the `or` operation (and corresponding `const` method) to allow
computing the union of a set of render layers. While we're here, also
added `and` and `xor` operations. Someone might find them useful

## Testing

Added a simple unit test.
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 C-Bug An unexpected or incorrect behavior C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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
Development

Successfully merging this pull request may close these issues.

7 participants