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 component to control UI camera position #5243

Closed
wants to merge 4 commits into from

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Jul 7, 2022

Objective

Enable user manipulation of UI camera.

Solution

Extend CameraUiConfig to include info about the UI camera position in
the "ui world". This allows fancy effects like moving UI, which was
possible before the migration to camera-driven rendering.

Note that the first commit of this PR (aa9593d) is part of PR #5234, only the second one is relevant.


Changelog

  • Add position and scale fields to CameraUiConfig to enable manipulating the position of the attached UI camera.

Migration Guide

  • Instead of spawning an individual ui camera with UiCameraBundle and manipulating its OrthographicProjection and GlobalTransform, add a CameraUiConfig component to your viewport camera, and use its field to change the ui camera position.

Comment on lines 327 to 338
if keyboard.pressed(KeyCode::A) {
config.position.x -= 1.0;
}
if keyboard.pressed(KeyCode::D) {
config.position.x += 1.0;
}
if keyboard.pressed(KeyCode::W) {
config.scale *= 0.99;
}
if keyboard.pressed(KeyCode::S) {
config.scale *= 1.01;
}
Copy link
Member

Choose a reason for hiding this comment

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

as an azerty user, I'm not so fan of WASD keys, maybe arrows would work ?
That would be more coherent with another example too : https://github.com/bevyengine/bevy/blob/main/examples/games/alien_cake_addict.rs#L202

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Living a mere 10 minutes drive from the French boundary, I'm a happy QWERTZ user :D my poor French colleagues always complaining about AZERTY support. Point taken though, I'll use arrow keys.

@@ -136,22 +134,50 @@ impl Default for ButtonBundle {
}
}
}
#[derive(Component, Clone)]
pub struct CameraUi {
pub is_enabled: bool,
Copy link
Member

Choose a reason for hiding this comment

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

So this wasn't used anymore? good spot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The meaning of CameraUi changed in #4745:

  • before: A marker component used to specify which camera was the one for rendering UI
  • after: A component to specify whether the camera it is attached to displays the UI (when not present, shows UI)

So technically still used, but for something almost opposite of what it used to.

I renamed it CameraUiConfig in #5234, because if you were previously relying on CameraUi, your game would still compile, but really do the opposite of what it was doing before.

I built on CameraUiConfig in this PR, because I thought it made sense as an API for manipulating the UI camera associated with actual 2d or 3d camera.

) {
for (entity, camera, config, render_info, config_changed) in query.iter_mut() {
if matches!(config, Some(&CameraUiConfig { show_ui: false, .. })) {
commands.entity(entity).remove::<UiCameraRenderInfo>();
Copy link
Member

Choose a reason for hiding this comment

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

Not so fan of this removal ?
if a user:

  • changes something within UiCameraRenderInfo
  • then disable UI
  • then re-enables it

-> the changes will be lost due to https://github.com/bevyengine/bevy/pull/5243/files#diff-5215fb8ee76dd4e8c35bf7ef10e986e57f73be9652d33200891b6a5a6f9d30d9R177-R183

Copy link
Contributor Author

@nicopap nicopap Jul 7, 2022

Choose a reason for hiding this comment

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

The component is read-only outside of bevy_ui, I expose it so that it's possible to access the camera projection (for example to compute proper offset for mouse interactions)

But yeah, you are right. Between the moment the user modify CameraUiConfig and the next CoreStage::PostUpdate this component will be out of date (both in term of value and existence). Which I agree is a footgun.

I don't like this either. How would this better be expressed? It needs both access to CameraUiConfig and Camera to work, so it's not as simple as replacing the content of CameraUiConfig.

Edit: Would a WorldQuery derive help? It could bundle a Camera and CameraUiConfig component access.

crates/bevy_ui/src/entity.rs Outdated Show resolved Hide resolved
@Nilirad Nilirad added C-Bug An unexpected or incorrect behavior A-UI Graphical user interfaces, styles, layouts, and widgets C-Regression Functionality that used to work but no longer does. Add a test for this! labels Jul 7, 2022
In bevy 0.7, `CameraUi` was a component specifically added to cameras
that display the UI. Since camera-driven rendering was merged, it
actually does the opposite! This will make it difficult for current
users to adapt to 0.8.

To avoid unnecessary confusion, we rename `CameraUi` into
`CameraUiConfig`.
Extend `CameraUiConfig` to include info about the UI camera position in
the "ui world". This allows fancy effects like moving UI, which was
possible before the migration to camera-driven rendering.

This reverts the regression caused by bevyengine#4765 preventing users from moving
the UI camera.
@nicopap nicopap marked this pull request as draft July 7, 2022 10:52
Having them separate caused update delay, so we remove them.
@nicopap
Copy link
Contributor Author

nicopap commented Jul 8, 2022

I've looked into implementing multiple ui camera handling. After considering the design for it, I'm not so sure this PR is the best way forward, there are alternative API design possible, and I need to explore them.

This design creates a new component to attach to existing camera entities. That component would contain all information pertaining to the UI camera.

Another option is for that component to just contain an Entity, a pointer to a different camera that will be used. The component might be an enum, with Disabled, Default, Custom(Entity) variants. The current implementation hints that this might be what cart intended.

Both have positives and negatives. But I'll have to explore how to implement both before reaching any conclusion.

@nicopap nicopap mentioned this pull request Jul 8, 2022
@nicopap
Copy link
Contributor Author

nicopap commented Jul 8, 2022

Closed in favor of #5252

@nicopap nicopap closed this Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI camera flexibility regression
3 participants