[Merged by Bors] - use marker components for cameras instead of name strings #3635

jakobhellermann · 2022-01-10T21:32:35Z

Problem

whenever you want more than one of the builtin cameras (for example multiple windows, split screen, portals), you need to add a render graph node that executes the correct sub graph, extract the camera into the render world and add the correct RenderPhase<T> components
querying for the 3d camera is annoying because you need to compare the camera's name to e.g. CameraPlugin::CAMERA_3d

Solution

Introduce the marker types Camera3d, Camera2d and CameraUi
-> Query<&mut Transform, With<Camera3d>> works
PerspectiveCameraBundle::new_3d() and PerspectiveCameraBundle::<Camera3d>::default() contain the Camera3d marker
OrthographicCameraBundle::new_3d() has Camera3d, OrthographicCameraBundle::new_2d() has Camera2d
remove ActiveCameras, ExtractedCameraNames
run 2d, 3d and ui passes for every camera of their respective marker
-> no custom setup for multiple windows example needed

Open questions

do we need a replacement for ActiveCameras? What about a component ActiveCamera { is_active: bool } similar to Visibility?

alice-i-cecile · 2022-01-10T22:08:52Z

Closes #1854. Previously attempted in #1888.

crates/bevy_core_pipeline/src/lib.rs

alice-i-cecile · 2022-01-10T22:10:35Z

We should think about how to coordinate this with #3412. @HackerFoo, do you have a preference?

alice-i-cecile · 2022-01-10T22:14:26Z

crates/bevy_render/src/camera/bundle.rs


 /// Component bundle for camera entities with perspective projection
 ///
 /// Use this for 3D rendering.
 #[derive(Bundle)]
-pub struct PerspectiveCameraBundle {
+pub struct PerspectiveCameraBundle<M: Component> {


I love this style of API; I've used it before in my games and it's great.

alice-i-cecile · 2022-01-10T22:15:41Z

crates/bevy_render/src/camera/bundle.rs

    pub fn new_3d() -> Self {
-        Default::default()
+        PerspectiveCameraBundle::new()


This feels a bit weird to me? Why not just use PerspectiveCameraBundle<Camera3d>::default() instead? And then specialize the default impls based on which generic is passed in.

I wanted to have only one Default impl so that PerspectiveCameraBundle { ..Default::default() } works without complaining about an uninferred type parameter.

HackerFoo · 2022-01-10T22:19:15Z

We should think about how to coordinate this with #3412. @HackerFoo, do you have a preference?

I'd like #3412 and #3568 to be considered, because they make it useful to run the draw_3d_graph subgraph on multiple cameras. It would be useful to attach some identifier to the types, to somehow control the layering order, and make other minor modifications to passes. I might have more feedback after reading through this PR.

I also have an idea for specifying the pass order in Camera, to allow e.g. skyboxes (behind everything else) and UI layers (in front.)

jakobhellermann · 2022-01-10T22:19:40Z

We should think about how to coordinate this with #3412. HackerFoo, do you have a preference?

Rebasing one on top of the other once it is merged shouldnt be too hard, the changes are in similar code paths but are mostly unrelated.

alice-i-cecile · 2022-01-10T22:21:03Z

This is looking much cleaner. I'm really happy to see this finally get cleaned up. Just a few nits :)

superdump · 2022-01-10T22:29:25Z

do we need a replacement for ActiveCameras? What about a component ActiveCamera { is_active: bool } similar to Visibility?

And here’s another instance of the same question. I’ve seen this 4 times in the past two days. :) @mockersf suggested adding enable/disable for lights while I was doing some other things but it’s not trivial imo because of the question of whether to have something like a ZST for the less likely case (i.e. an empty InactiveCamera component) which introduces archetype fragmentation but is directly queryable, or always adding an ActiveCamera { is_active: bool } component and forcing visiting all entities just to find one or the other subset, and always using the storage for the component. @cart suggested adding Visibility to the light bundles but Visibility feels like the wrong property for lights. I felt like something more generically usable everywhere like Enabled { is_enabled: bool } or Active { is_active: bool } or some better name, for all cases. Visibility, lights being enabled, cameras being active, whatever. And then when we make an editor there is one component that handles them all in a sensible and consistent way. But, I didn’t proceed because there’s still the question of the component with a bool member or the marker component for the less likely case. It feels like marker components should be used only where iteration performance is a top priority because I fear the combinatoric explosion and subsequent fragmentation if we use too many marker components… :/ I don’t know

alice-i-cecile · 2022-01-10T22:38:16Z

I felt like something more generically usable everywhere like Enabled { is_enabled: bool } or Active { is_active: bool } or some better name, for all cases

I'm in favor of this: being able to standardize the ecosystem around this pattern is super useful.

But, I didn’t proceed because there’s still the question of the component with a bool member or the marker component for the less likely case. It feels like marker components should be used only where iteration performance is a top priority because I fear the combinatoric explosion and subsequent fragmentation if we use too many marker components… :/

So, IMO archetype fragmentation is less important performance-wise than the requirement to constantly check values. I'd prefer a marker component: if we use sparse-set storage it'll be faster to add and remove too. I would be fine with either Enabled/Active or Disabled/Inactive as the marker, depending on which pattern you think is clearer / more useful for your rendering use cases.

Overall I think a Inactive marker component is going to be the clearest for debugging purposes, and the most natural to reuse in other contexts. I'd probably toss it in bevy_ecs; it'll be reused all over the place.

HackerFoo · 2022-01-10T22:43:58Z

How about RenderLayers::none() to disable a camera? That should already work, but maybe isn't optimized. I like RenderLayers because you can define complex relations between cameras and renderable entities.

Similar mechanisms are used for physics (e.g. in Rapier.)

HackerFoo · 2022-01-10T22:47:58Z

It would be useful to attach some identifier to the types

I suppose this could be done with some other component if needed.

superdump · 2022-01-11T20:35:51Z

I felt like something more generically usable everywhere like Enabled { is_enabled: bool } or Active { is_active: bool } or some better name, for all cases

I'm in favor of this: being able to standardize the ecosystem around this pattern is super useful.

👍

But, I didn’t proceed because there’s still the question of the component with a bool member or the marker component for the less likely case. It feels like marker components should be used only where iteration performance is a top priority because I fear the combinatoric explosion and subsequent fragmentation if we use too many marker components… :/

So, IMO archetype fragmentation is less important performance-wise than the requirement to constantly check values. I'd prefer a marker component: if we use sparse-set storage it'll be faster to add and remove too. I would be fine with either Enabled/Active or Disabled/Inactive as the marker, depending on which pattern you think is clearer / more useful for your rendering use cases.

I didn’t know this so I hoped those who know more about the ECS performance impacts would speak up so thanks! :)

Overall I think a Inactive marker component is going to be the clearest for debugging purposes, and the most natural to reuse in other contexts. I'd probably toss it in bevy_ecs; it'll be reused all over the place.

Sounds good to me. I generally prefer to have positive logic in the sense of avoiding thinking “if !thing {} else {}” but in these marker component cases it feels like, from an ECS perspective, it makes more sense to mark the less frequent case. That’s how I ended up with NotShadowCaster and NotShadowReceiver. Following that logic and assuming that relatively few entities would be disabled, marking as Inactive makes sense to me. :)

alice-i-cecile · 2022-01-11T21:20:28Z

Following that logic and assuming that relatively few entities would be disabled, marking as Inactive makes sense to me. :)

I think the strategy here is then:

Create the Inactive marker in bevy_ecs in this PR.
Swap all of the other rendering places where this could be used to Inactive as a seperate commit in this PR.
Discuss automatically excluding Inactive entities from queries by default in another issue / PR.

HackerFoo · 2022-01-11T21:28:57Z

It would be useful to be able wrap any component in Deactivatable<T: Component> (I know, terrible name) with a command to set if the component is active. Or maybe it could be a trait? That way it's similar to change detection, which works on all components, but it would be opt-in, unless it's cheap enough to implement for all components.

I suggest not tying it to this PR, though. The existing component RenderLayers should work fine for now, in my opinion.

alice-i-cecile · 2022-01-11T23:56:38Z

Yeah okay @HackerFoo is right: we should use the simplest possible solution for this PR, and then a better solution later.

HackerFoo · 2022-03-10T06:44:55Z

An exclusive marker component would be generally useful. Maybe it could be both a resource containing just an optional entity, and a component, where inserting it updates the resource and removes the component from the previous entity.

I've used both marker components and resources to identify specific entities. Marker components are more flexible and can't have invalid entities, but resources are probably faster and are guaranteed unique.

superdump · 2022-03-10T09:22:36Z

Another problem with marker components is that they require command updates and if you have system dependencies with a stage that’s a problem.

Why can there only be one camera of one type in a scene? What about split screen local for example?

cart · 2022-03-10T23:37:17Z

Why can there only be one camera of one type in a scene? What about split screen local for example?

There can only be one active camera of one type per render target. Split screen would involve at least two render targets: either two targets with custom viewports or three targets .... two for each split and the third for compositing. Cameras will correlate to single render targets. "Exclusive marker components" wouldn't cover this case. "Non-exclusive marker components" would work, because you could iterate the active list and filter down to the target currently being rendered. "Exclusive resource" also wouldn't work for multiple render targets. My plan there was either to make the ActiveCamera<T> a resource for the "default target" and a component for "custom targets", or to make all targets entities (for consistency) and insert the default target entity by default.

cart · 2022-03-11T00:17:18Z

I'm pretty partial to this design: "every RenderTarget is an entity, every RenderTarget has ActiveCamera<T>(Option<Entity>), which defaults to the first camera in the query, if none is set". It provides solid zero-user-effort default behavior and also provides a single, easy-to-query, canonical place to find and set each target's active camera. This would even still work with scenes, provided the RenderTarget added to a scene file can reference the active camera (which is currently possible for scenes + will
ultimately be possible for nested scenes as well). This seems like a solid tradeoff, given the distributed / nestable nature of scenes.

Given that right now RenderTarget entities aren't a thing (im working on those now), I think building a hard-coded ActiveCamera<T> resource first in this PR, then migrating to components + entities later is the way to go. I'll get started on that now, but feel free to continue discussing designs.

HackerFoo · 2022-03-11T06:43:21Z

There can only be one active camera of one type per render target.

The example in #3552 uses two cameras with the same render target (RenderTarget::Window(WindowId::primary()).) Furthermore, my app (Noumenal) uses two cameras in order to layer the 3D components of the UI on top.

As further motivation, I've considered updating the cameras at different rates (the first is mostly static without user interaction), so I've been thinking about how to store the colors from the first pass and only rendering the second. I could use compositing, but passes are expensive on mobile devices - I saved ~30% GPU time by just eliminating the clear pass, and a bunch more by eliminating other passes.

Nonetheless, rendering multiple cameras directly to a single texture has been useful for me in practice.

cart · 2022-03-11T22:01:47Z

Nonetheless, rendering multiple cameras directly to a single texture has been useful for me in practice.

This will still be possible with the outlined plan, as RenderTargets won't be "exclusive". You can create a second RenderTarget that also targets WindowId::primary(), then assign your second camera (of the same type) to it. They can both be "active", they will just be active in different targets.

I like this design because it embraces the fact that the core pipeline is designed around "one active camera of a given type at a time", and lets us treat cameras as "simple logical views" instead of trying to treat them as "owners of render pass configuration".

cart · 2022-03-11T22:48:41Z

Just added an ActiveCamera<T> resource and CameraTypePlugin<T> (for easily adding new camera types). I think this is probably the right general direction, but I'm down to continue discussing the cameras/passes/render_targets relationships more. I do think we should merge the ActiveCamera approach because it is a "lateral move" ... it doesn't change the current paradigm, it just adapts it to use marker components. That way we can merge this PR asap without worrying too much about breaking people, and we can start building new features on top of marker components.

alice-i-cecile

I would prefer basic docs throughout this PR, but I can live without them and add them in a follow-up.

alice-i-cecile

I think this a good approach, and the code quality is good. LGTM.

crates/bevy_render/src/camera/camera.rs

examples/3d/render_to_texture.rs

cart · 2022-03-12T00:40:50Z

bors r+

**Problem** - whenever you want more than one of the builtin cameras (for example multiple windows, split screen, portals), you need to add a render graph node that executes the correct sub graph, extract the camera into the render world and add the correct `RenderPhase<T>` components - querying for the 3d camera is annoying because you need to compare the camera's name to e.g. `CameraPlugin::CAMERA_3d` **Solution** - Introduce the marker types `Camera3d`, `Camera2d` and `CameraUi` -> `Query<&mut Transform, With<Camera3d>>` works - `PerspectiveCameraBundle::new_3d()` and `PerspectiveCameraBundle::<Camera3d>::default()` contain the `Camera3d` marker - `OrthographicCameraBundle::new_3d()` has `Camera3d`, `OrthographicCameraBundle::new_2d()` has `Camera2d` - remove `ActiveCameras`, `ExtractedCameraNames` - run 2d, 3d and ui passes for every camera of their respective marker -> no custom setup for multiple windows example needed **Open questions** - do we need a replacement for `ActiveCameras`? What about a component `ActiveCamera { is_active: bool }` similar to `Visibility`? Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-03-12T00:55:04Z

Pull request successfully merged into main.

Build succeeded:

The example was broken in #3635 when the `ActiveCamera` logic was introduced, after which there could only be one active `Camera3d` globally. Ideally there could be one `Camera3d` per render target, not globally, but that isn't the case yet. To fix the example, we need to - don't use `Camera3d` twice, add a new `SecondWindowCamera3d` marker - add the `CameraTypePlugin::<SecondWindowCamera3d>` - extract the correct `RenderPhase`s - add a 3d pass driver node for the secondary camera Fixes #4378 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

…#3635) **Problem** - whenever you want more than one of the builtin cameras (for example multiple windows, split screen, portals), you need to add a render graph node that executes the correct sub graph, extract the camera into the render world and add the correct `RenderPhase<T>` components - querying for the 3d camera is annoying because you need to compare the camera's name to e.g. `CameraPlugin::CAMERA_3d` **Solution** - Introduce the marker types `Camera3d`, `Camera2d` and `CameraUi` -> `Query<&mut Transform, With<Camera3d>>` works - `PerspectiveCameraBundle::new_3d()` and `PerspectiveCameraBundle::<Camera3d>::default()` contain the `Camera3d` marker - `OrthographicCameraBundle::new_3d()` has `Camera3d`, `OrthographicCameraBundle::new_2d()` has `Camera2d` - remove `ActiveCameras`, `ExtractedCameraNames` - run 2d, 3d and ui passes for every camera of their respective marker -> no custom setup for multiple windows example needed **Open questions** - do we need a replacement for `ActiveCameras`? What about a component `ActiveCamera { is_active: bool }` similar to `Visibility`? Co-authored-by: Carter Anderson <mcanders1@gmail.com>

The example was broken in bevyengine#3635 when the `ActiveCamera` logic was introduced, after which there could only be one active `Camera3d` globally. Ideally there could be one `Camera3d` per render target, not globally, but that isn't the case yet. To fix the example, we need to - don't use `Camera3d` twice, add a new `SecondWindowCamera3d` marker - add the `CameraTypePlugin::<SecondWindowCamera3d>` - extract the correct `RenderPhase`s - add a 3d pass driver node for the secondary camera Fixes bevyengine#4378 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

…#3635) **Problem** - whenever you want more than one of the builtin cameras (for example multiple windows, split screen, portals), you need to add a render graph node that executes the correct sub graph, extract the camera into the render world and add the correct `RenderPhase<T>` components - querying for the 3d camera is annoying because you need to compare the camera's name to e.g. `CameraPlugin::CAMERA_3d` **Solution** - Introduce the marker types `Camera3d`, `Camera2d` and `CameraUi` -> `Query<&mut Transform, With<Camera3d>>` works - `PerspectiveCameraBundle::new_3d()` and `PerspectiveCameraBundle::<Camera3d>::default()` contain the `Camera3d` marker - `OrthographicCameraBundle::new_3d()` has `Camera3d`, `OrthographicCameraBundle::new_2d()` has `Camera2d` - remove `ActiveCameras`, `ExtractedCameraNames` - run 2d, 3d and ui passes for every camera of their respective marker -> no custom setup for multiple windows example needed **Open questions** - do we need a replacement for `ActiveCameras`? What about a component `ActiveCamera { is_active: bool }` similar to `Visibility`? Co-authored-by: Carter Anderson <mcanders1@gmail.com>

The example was broken in bevyengine#3635 when the `ActiveCamera` logic was introduced, after which there could only be one active `Camera3d` globally. Ideally there could be one `Camera3d` per render target, not globally, but that isn't the case yet. To fix the example, we need to - don't use `Camera3d` twice, add a new `SecondWindowCamera3d` marker - add the `CameraTypePlugin::<SecondWindowCamera3d>` - extract the correct `RenderPhase`s - add a 3d pass driver node for the secondary camera Fixes bevyengine#4378 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>

jakobhellermann added the A-Rendering Drawing game state to the screen label Jan 10, 2022

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 10, 2022

jakobhellermann added C-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use labels Jan 10, 2022

james7132 removed the S-Needs-Triage This issue needs to be labelled label Jan 10, 2022

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change and removed C-Enhancement A new feature labels Jan 10, 2022

alice-i-cecile reviewed Jan 10, 2022

View reviewed changes

crates/bevy_core_pipeline/src/lib.rs Outdated Show resolved Hide resolved

alice-i-cecile requested a review from HackerFoo January 10, 2022 22:10

alice-i-cecile reviewed Jan 10, 2022

View reviewed changes

alice-i-cecile added the S-Needs-Migration-Guide label Jan 10, 2022

HackerFoo approved these changes Jan 10, 2022

View reviewed changes

This was referenced Jan 15, 2022

Auto add default UI camera #3679

Closed

Add marker components for cameras #1888

Closed

alice-i-cecile approved these changes Jan 18, 2022

View reviewed changes

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 Jan 18, 2022

cart added 2 commits March 10, 2022 15:56

fix render_to_texture

00da121

clippy

03cf004

ActiveCamera resource and CameraTypePlugin

124138d

alice-i-cecile reviewed Mar 11, 2022

View reviewed changes

alice-i-cecile approved these changes Mar 11, 2022

View reviewed changes

james7132 requested changes Mar 11, 2022

View reviewed changes

crates/bevy_render/src/camera/camera.rs Outdated Show resolved Hide resolved

examples/3d/render_to_texture.rs Show resolved Hide resolved

cart added 2 commits March 11, 2022 15:27

only extract active cameras

b6cd8c9

extract active first pass camera

979cdb9

james7132 approved these changes Mar 12, 2022

View reviewed changes

bors bot changed the title ~~use marker components for cameras instead of name strings~~ [Merged by Bors] - use marker components for cameras instead of name strings Mar 12, 2022

bors bot closed this Mar 12, 2022

Azorlogh mentioned this pull request Mar 17, 2022

Ability to extract any number of cameras #4239

Closed

kirusfg mentioned this pull request Mar 18, 2022

Panic after despawning camera #4227

Closed

jakobhellermann deleted the camera-marker-components branch March 31, 2022 17:51

jakobhellermann mentioned this pull request Apr 1, 2022

[Merged by Bors] - fix multiple_windows example #4389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - use marker components for cameras instead of name strings #3635

[Merged by Bors] - use marker components for cameras instead of name strings #3635

jakobhellermann commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

alice-i-cecile Jan 10, 2022

alice-i-cecile Jan 10, 2022

jakobhellermann Jan 11, 2022

HackerFoo commented Jan 10, 2022

jakobhellermann commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

superdump commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022 •

edited

Loading

HackerFoo commented Jan 10, 2022 •

edited

Loading

HackerFoo commented Jan 10, 2022 •

edited

Loading

superdump commented Jan 11, 2022

alice-i-cecile commented Jan 11, 2022

HackerFoo commented Jan 11, 2022 •

edited

Loading

alice-i-cecile commented Jan 11, 2022

HackerFoo commented Mar 10, 2022

superdump commented Mar 10, 2022

cart commented Mar 10, 2022

cart commented Mar 11, 2022 •

edited

Loading

HackerFoo commented Mar 11, 2022

cart commented Mar 11, 2022

cart commented Mar 11, 2022 •

edited

Loading

alice-i-cecile left a comment

alice-i-cecile left a comment

cart commented Mar 12, 2022

bors bot commented Mar 12, 2022

[Merged by Bors] - use marker components for cameras instead of name strings #3635

[Merged by Bors] - use marker components for cameras instead of name strings #3635

Conversation

jakobhellermann commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

alice-i-cecile Jan 10, 2022

Choose a reason for hiding this comment

alice-i-cecile Jan 10, 2022

Choose a reason for hiding this comment

jakobhellermann Jan 11, 2022

Choose a reason for hiding this comment

HackerFoo commented Jan 10, 2022

jakobhellermann commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022

superdump commented Jan 10, 2022

alice-i-cecile commented Jan 10, 2022 • edited Loading

HackerFoo commented Jan 10, 2022 • edited Loading

HackerFoo commented Jan 10, 2022 • edited Loading

superdump commented Jan 11, 2022

alice-i-cecile commented Jan 11, 2022

HackerFoo commented Jan 11, 2022 • edited Loading

alice-i-cecile commented Jan 11, 2022

HackerFoo commented Mar 10, 2022

superdump commented Mar 10, 2022

cart commented Mar 10, 2022

cart commented Mar 11, 2022 • edited Loading

HackerFoo commented Mar 11, 2022

cart commented Mar 11, 2022

cart commented Mar 11, 2022 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

cart commented Mar 12, 2022

bors bot commented Mar 12, 2022

alice-i-cecile commented Jan 10, 2022 •

edited

Loading

HackerFoo commented Jan 10, 2022 •

edited

Loading

HackerFoo commented Jan 10, 2022 •

edited

Loading

HackerFoo commented Jan 11, 2022 •

edited

Loading

cart commented Mar 11, 2022 •

edited

Loading

cart commented Mar 11, 2022 •

edited

Loading