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

Add marker components for cameras #1888

Closed
wants to merge 1 commit into from
Closed

Add marker components for cameras #1888

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 11, 2021

Adds marker components for cameras, as suggested in #1854.
Fixes #1854.

Changes alien cake addict example to use the GameplayCamera marker in order to show use.

/// Marker component used in [PerspectiveCameraBundle] and
/// [OrthographicCameraBundle].
#[derive(Default)]
pub struct GameplayCamera;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the name GameplayCamera.

I think MainCamera would be better, considering that we have the MainPass marker component for entities that are to be rendered in the "main pass", which is where this camera is used.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I don't mind one way or the other, I think your reasoning is good though so I can change it to MainCamera.

Will ping @alice-i-cecile though as the original naming came from #1854 .
Any comments?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid MainCamera: it becomes very confusing with multiple viewports when you have multiple cameras, all of which have the MainCamera marker component.

The idea to communicate here is "renders objects in the game world, not the UI". WorldCamera might also makes sense, and is less game-specific, but given that UI entities are also going to be stored in the World it could be confusing as well.

@alice-i-cecile alice-i-cecile added core C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Apr 11, 2021
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.

IMO we should remove the name field from Camera as part of this PR as well.

@ghost
Copy link
Author

ghost commented Apr 12, 2021

IMO we should remove the name field from Camera as part of this PR as well.

Ok, I started on this.
But I think I will have to study the code (and/or get some reviewer comments) on what to do with bevy_render/src/camera/active_cameras.rs if the name field goes away.

That system is based around &str access to cameras.
ActiveCameras again is referenced in a few places: render_graph, bevy_sprite, bevy_ui, and the multiple windows example.

It sounds very manageable with a little time, but please give me some input on what the best way forward is:

  1. Split removing the name field into another issue
  2. Rework active_cameras.rs to not be "stringly typed", and adapt all usages to follow
  3. Remove active_camera.rs altogether? Maybe it isn't needed when markers are available

@alice-i-cecile
Copy link
Member

Taking a closer look @Grindv1k, I think that removing the name field deserves to be part of a follow-up issue and PR. active_camera gets plumbed around in a number of places, and won't be a trivial fix.

This solves the end-user case; we can do code-quality clean-up on the internals separately, particularly since the rendering is due for a rework.

@cart
Copy link
Member

cart commented Apr 14, 2021

Camera names are also "end user" apis. I do think marker components and names solve very similar problems and having both feels a bit odd. I think marker components are the right move, but I'd want a concrete plan before committing to them. Prior to adding marker components, I think we should have either:

  1. a complete replacement of camera names in this pr
  2. a merged RFC illustrating a clear plan for the "marker only" future

While we wait, users can already grab the entity for a given camera name using ActiveCameras, then plug that into camera queries.

Signed-off-by: Torstein Grindvik <torstein.grindvik@gmail.com>
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@alice-i-cecile
Copy link
Member

Closing in favor of #3635 <3 Thanks for all the exploration you did here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add marker components for different camera types
5 participants