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

Rendering masks #1209

Merged
merged 13 commits into from
Jan 8, 2021
Merged

Rendering masks #1209

merged 13 commits into from
Jan 8, 2021

Conversation

schell
Copy link
Contributor

@schell schell commented Jan 5, 2021

tl;dr adds RenderLayers to link groups of entities to cameras.

This describes which rendering layers an entity belongs to. A layer is a collection of entities that only get rendered by cameras in that layer.

Entities and cameras without a RenderLayers component can be considered to belong to the "default" layer (layer 0), which is the same as entities and cameras with the component RenderLayers::default().

Entities and cameras can belong to multiple layers:

commands
    .spawn(...)
    .with(
        RenderLayers::layer(0) // starts with just layer 0
            .with(1) // also sets layer 1
    )

Using a mask of u32 we have 32 possible rendering layers, 0-31 plus the empty mask RenderLayers::none().

More takeaways:

  • by default, entities belong to layer 0
  • entities without a mask belong to layer 0
  • entities with a mask without any layers (RenderLayers::none()) are invisible, as if they had the component Visible { is_visible: false }

@cart
Copy link
Member

cart commented Jan 7, 2021

I dig this. I only have one comment:

With the current masking approach (empty is default, empty matches empty) I don't think there is a way to encode the intent "draw everything". If you set all of the bits in the mask to 1, it will draw everything but the default empty mask. And if you set all of the bits to 0, it will only draw things that are also 0. I think it makes more sense to default to "group 1" and implicitly assume things without the mask component are also "group 1". This has the added benefit of being the behavior used in Godot, which makes it more familiar. This would mean camera masks would need to default to "group 1" (or maybe all groups, which is what godot does).

@schell
Copy link
Contributor Author

schell commented Jan 7, 2021

@cart, @mockersf, @DJMcNab I've updated the PR with your suggestions and then some (docs, tests) and updated the PR desc.

crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Looks very good, just a few code style nits/duplications for me.

///
/// The [`Default`] instance of `RenderLayers` contains layer `0`, the first layer.
///
/// An entity with this component without any layers is invisible.
Copy link
Member

@DJMcNab DJMcNab Jan 8, 2021

Choose a reason for hiding this comment

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

I'm still cautious about making this guarantee, since we still seem to require that every entity has a Visible, which this overlaps with.

That being said, I don't think there is necessarily better behaviour. Maybe we could have a warning log which prints once (i.e. if any entity matches this, then print and set an AtomicBool to true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a bit sketchy but it does help describe the feature. I think this is the best we can do at the moment. I personally plan on using the "invisiblity" of RenderLayers::none() so I don't think we should add a warning.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I thought a bit about just replacing Visible with RenderLayers. But I think we need a "global" visible/invisible toggle:

  • That way visibility can still be toggled without needing to know what layers the current entity is visible on
  • Visibility will likely eventually propagate down a hierarchy, while RenderLayers might not. Its worth discussing at some point, but imo doesn't need to be settled in this pr.
  • I want entity visibility to be easily toggle-able in the editor from the "Entity Inspector" (ex: have an "eye" toggle next to the entity name), which would be much harder for arbitrary layers

crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/camera/visible_entities.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Jan 8, 2021

Just left one last baby comment, then I think this is good to go. Very nicely done!

@cart cart merged commit a6a242c into bevyengine:master Jan 8, 2021
@schell schell deleted the rendering_masks branch January 8, 2021 21:53
rparrett pushed a commit to rparrett/bevy that referenced this pull request Jan 27, 2021
Adds RenderLayers, which enable cameras and entities to opt in to layers that apply to them
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants