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

[Merged by Bors] - Added multi windows check for bevy_ui Interaction. #5225

Closed
wants to merge 7 commits into from
Closed

[Merged by Bors] - Added multi windows check for bevy_ui Interaction. #5225

wants to merge 7 commits into from

Conversation

farend-east
Copy link
Contributor

@farend-east farend-east commented Jul 6, 2022

Objective

Solution

  • Added checks for focused windows in Interaction focus system.

Follow Up

  • All windows with camera will be rendering the UI elements right now.
  • We will need some way to tell which camera to render which UI.

`Interaction` focus system will get first available window's cursor position instead of just primary window.
@Nilirad Nilirad added A-Windowing Platform-agnostic interface layer to run your app in A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 6, 2022
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

There is a situation that this handles poorly:

You have two windows:

  • Window 1 with UI elements
  • Window 2 without the UI elements

When the mouse hovers over Window 2 I believe, this patch will activate the button under the corresponding position in Window 1.

I suggest this alternative: get the Window for windows.get(id) where id is the target: RenderTarget::Window(WindowId) field of the Camera component of the entity with the CameraUi marker component. Probably requires adding a query to ui_focus_system or store the ID in an additional resource.

This is more difficult, because you can have multiple ui cameras. I think currently, it would be fine to take the windows id of the first ui camera and keep the issue open. A design that accounts for all ui cameras might be way harder to handle properly.

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
Now the focus system will consider the currently focused window instead of all window
Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

I think we should limit the query to stuff with a CameraUi component. Otherwise, we might pick up other types of camera.

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
Comment on lines 97 to 102
if let Some(&CameraUi {
is_enabled: false, ..
}) = camera_ui
{
return None;
}
Copy link
Contributor

@nicopap nicopap Jul 6, 2022

Choose a reason for hiding this comment

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

With the change suggested on the query parameter, you can replace this with if !camera_ui.is_enabled.

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 user are able to add in any camera without this component.

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
@nicopap
Copy link
Contributor

nicopap commented Jul 6, 2022

Give me a minute and I'll give you a third review.

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

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

LGTM I'd like this merged

crates/bevy_ui/src/focus.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jul 7, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 8, 2022
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 mentioned this pull request Jul 8, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 8, 2022
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 added a commit to nicopap/bevy that referenced this pull request Jul 9, 2022
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.
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior and removed C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Jul 11, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 11, 2022
# Objective

- Currently bevy_ui only checks for primary window cursor position to determine `Interaction` behavior.
- Added checks for focused window where cursor position is available.
- Fixes #5224.

## Solution

- Added checks for focused windows in `Interaction` focus system.

## Follow Up

- All windows with camera will be rendering the UI elements right now.
- We will need some way to tell which camera to render which UI.

---

Co-authored-by: fadhliazhari <44402264+fadhliazhari@users.noreply.github.com>
@bors bors bot changed the title Added multi windows check for bevy_ui Interaction. [Merged by Bors] - Added multi windows check for bevy_ui Interaction. Jul 11, 2022
@bors bors bot closed this Jul 11, 2022
@farend-east farend-east deleted the multiple-window-ui-interaction branch July 12, 2022 08:31
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 19, 2022
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 added a commit to nicopap/bevy that referenced this pull request Jul 21, 2022
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 mentioned this pull request Jul 21, 2022
nicopap added a commit to nicopap/bevy that referenced this pull request Jul 21, 2022
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.
inodentry pushed a commit to IyesGames/bevy that referenced this pull request Aug 8, 2022
# Objective

- Currently bevy_ui only checks for primary window cursor position to determine `Interaction` behavior.
- Added checks for focused window where cursor position is available.
- Fixes bevyengine#5224.

## Solution

- Added checks for focused windows in `Interaction` focus system.

## Follow Up

- All windows with camera will be rendering the UI elements right now.
- We will need some way to tell which camera to render which UI.

---

Co-authored-by: fadhliazhari <44402264+fadhliazhari@users.noreply.github.com>
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Currently bevy_ui only checks for primary window cursor position to determine `Interaction` behavior.
- Added checks for focused window where cursor position is available.
- Fixes bevyengine#5224.

## Solution

- Added checks for focused windows in `Interaction` focus system.

## Follow Up

- All windows with camera will be rendering the UI elements right now.
- We will need some way to tell which camera to render which UI.

---

Co-authored-by: fadhliazhari <44402264+fadhliazhari@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Currently bevy_ui only checks for primary window cursor position to determine `Interaction` behavior.
- Added checks for focused window where cursor position is available.
- Fixes bevyengine#5224.

## Solution

- Added checks for focused windows in `Interaction` focus system.

## Follow Up

- All windows with camera will be rendering the UI elements right now.
- We will need some way to tell which camera to render which UI.

---

Co-authored-by: fadhliazhari <44402264+fadhliazhari@users.noreply.github.com>
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 A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow bevy_ui to handle multiple window
6 participants