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

User UI camera control #5252

Closed
wants to merge 6 commits into from
Closed

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jul 8, 2022

Objective

Enable user manipulation of UI camera.

Solution

Add user control of UI cameras

Add fields to UiCameraConfig to control the UI camera scale and
position.

Fixes #5242. It is again possible to manipulate the ui camera.

An alternative design was considered, where instead of having a
component that controls all of the UI camera settings, the component
would only hold an Entity referencing another camera.

That design was abandoned in favor of the current one because the
viewport is tightly bound to the "actual" camera the UI camera is
attached to. So it would be awkward to maintain independently two
different cameras.


Changelog

  • Add position, scale and window_origin fields to UiCameraConfig to enable
    manipulating the position of the attached UI camera.
  • Add UI Camera control example
  • Fix UI focus not working properly with moving cameras

Migration Guide

  • Instead of spawning an individual ui camera with UiCameraBundle
    and manipulating its OrthographicProjection and GlobalTransform,
    add a UiCameraConfig component to your viewport camera, and
    use its field to change the ui camera position
  • Use the UiCameraProjection component to access the UI camera's projection.

@alice-i-cecile
Copy link
Member

Sibling to #5114 and #4007.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets labels Jul 8, 2022
@nicopap nicopap force-pushed the multi-uicam-target branch from abea0a4 to 3b603cb Compare July 8, 2022 18:15
@nicopap nicopap marked this pull request as ready for review July 8, 2022 18:16
@nicopap nicopap force-pushed the multi-uicam-target branch from 3b603cb to 3072955 Compare July 9, 2022 17:38
@nicopap nicopap force-pushed the multi-uicam-target branch from 3072955 to 00655b2 Compare July 19, 2022 14:07
@alice-i-cecile alice-i-cecile added this to the Bevy 0.8 milestone Jul 20, 2022
@alice-i-cecile
Copy link
Member

@IceSentry @superdump, I think this is a serious enough breakage that we should try to get this reviewed and merged for 0.8

@nicopap
Copy link
Contributor Author

nicopap commented Jul 20, 2022

I'd also like to get cart's input on it. Since from reading the code it gave out the impression he had something in mind.

#[derive(Component)]
pub struct DefaultCameraView(pub Entity);
#[derive(Component, Debug)]
pub struct UiCamera {
Copy link
Member

Choose a reason for hiding this comment

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

Docs needed.

..default()
},
})
.insert(UiCameraConfig {
Copy link
Member

Choose a reason for hiding this comment

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

This part of the example is unclear, and needs more comments.

We can also probably shorten it now that #5343 has merged!

@@ -309,6 +316,31 @@ struct ScrollingList {
position: f32,
}

fn change_ui_camera(
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should have this in its own example. I don't like how big this example is to begin with; let's not make it worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, the off_screen_focusables from ui-navigation seems like a perfect fit, and would help lower the size of the rfc41 PR too.

@IceSentry
Copy link
Contributor

I feel like this should be split in 2 PR. I like the render layer idea, but I don't like the config part because of the pub projection.

@nicopap
Copy link
Contributor Author

nicopap commented Jul 20, 2022

@IceSentry said:

I feel like this should be split in 2 PR. I like the render layer idea, but I don't like the config part because of the pub projection.

Indeed. In insight, it's quite obvious those are two different features.

The RenderLayer addition however, leaves #5242 unsolved. I agree the pub projection is questionable, and I've spent a some time (let's be honest, few minutes) thinking about it.

The thing is, you need access to the UI projection for a lot of reasons (navigation is one of them).

Here is a different proposition: have a UiProjection component which public API provide read-only access to the projection, have a system to update it based on the associated camera and the config. I'm still very annoyed by that solution, because you then have to add systems relying on the projection to a specific stage and make sure they run .after the ui projection update system.

But the one I provided here suffers from the same issue, so maybe it's OK?

@alice-i-cecile
Copy link
Member

Mhmm. I think we split this, merge the RenderLayers part ASAP, and then chew on the config in this PR.

@nicopap
Copy link
Contributor Author

nicopap commented Jul 20, 2022

Mhmm. I think we split this, merge the RenderLayers part ASAP, and then chew on the config in this PR.

Can do, ready tomorrow. But if it doesn't fix #5242 There is no reason for it to be a priority anymore.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.8 milestone Jul 20, 2022
@nicopap nicopap force-pushed the multi-uicam-target branch from 387e95f to be9d099 Compare July 21, 2022 10:09
@nicopap nicopap marked this pull request as draft July 21, 2022 10:09
nicopap added 2 commits July 21, 2022 13:46
This enables having different UI per camera. With bevyengine#5225, this enables
having different interactive UIs per window. Although, to properly
complete this, the focus system will need to account for RenderLayer of
UI nodes.
@nicopap nicopap force-pushed the multi-uicam-target branch from be9d099 to 60c29b8 Compare July 21, 2022 11:52
@nicopap nicopap changed the title User UI camera control & UI element layer support User UI camera control Jul 21, 2022
nicopap added 3 commits July 21, 2022 15:01
Add fields to UiCameraConfig to control the UI camera scale and
window_origin.

Fixes bevyengine#5242. It is again possible to manipulate the ui camera.

An alternative design was considered, where instead of having a
component that controls all of the UI camera settings, the component
would only hold an Entity referencing another camera.

That design was abandoned in favor of the current one because the
viewport is tightly bound to the "actual" camera the UI camera is
attached to. So it would be awkward to maintain independently two
different cameras.
@nicopap nicopap force-pushed the multi-uicam-target branch from 60c29b8 to 90d4cb8 Compare July 21, 2022 14:07
@nicopap nicopap marked this pull request as ready for review July 21, 2022 14:15
@nicopap nicopap marked this pull request as draft July 21, 2022 14:16
@nicopap
Copy link
Contributor Author

nicopap commented Jul 21, 2022

I created a separate PR for the RenderLayers, see #5414. Note that this PR still contains all the commits from #5414, so I expect this PR to be reviewed later on.

nicopap added a commit to nicopap/bevy that referenced this pull request Jul 21, 2022
@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jul 21, 2022
@nicopap nicopap mentioned this pull request Jul 21, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 22, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 31, 2022
@nicopap nicopap removed the S-Blocked This cannot move forward until something else changes label Aug 7, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Aug 9, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Aug 18, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Sep 4, 2022
@nicopap
Copy link
Contributor Author

nicopap commented Sep 25, 2022

This PR is not fully addressed by #5892, but I think it should be the direction forward, thus superseding this PR.

@nicopap nicopap closed this Sep 25, 2022
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 A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI camera flexibility regression
3 participants