Skip to content

Correctly handle UI hierarchy without a camera #12816

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
merged 7 commits into from
Apr 22, 2024

Conversation

salvadorcarvalhinho
Copy link
Contributor

Objective

Add support so bevy_ui can correctly handle an UI hierarchy without a camera present.

Solution

As there was no default behavior for what should happen when a camera is not present in a UI hierarchy, the solution
was based in defining that default behavior and improving the overall handling of this "exception".

Changelog

  • Create default values to be used in upsert_node
  • Add flag to control warnings about no camera present
  • Create unit test no_camera_ui (to test if ui handles no camera present)

Created default values to be used in upsert_node, added flag to control warnings about no camera present and created unit test no_camera_ui.
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@NthTensor NthTensor added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Mar 31, 2024
let layout_info = camera_layout_info
.entry(camera_entity)
.or_insert_with(|| calculate_camera_layout_info(camera));
layout_info.root_nodes.push(entity);
}
None => {
if cameras.is_empty() {
warn!("No camera found to render UI to. To fix this, add at least one camera to the scene.");
if !ui_surface.no_camera {
ui_surface.no_camera = true;
Copy link
Contributor

@StrikeForceZero StrikeForceZero Mar 31, 2024

Choose a reason for hiding this comment

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

IMO the no_camera flag is probably not needed and adds an extra layer of state to manage. I tested it with this diff below applied to your branch and it still passed the test. But the else upsert_node below with the default style and layout is a good catch. 👍

If we are worried about spamming the console we could use a warn_once! instead.

Index: crates/bevy_ui/src/layout/mod.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/crates/bevy_ui/src/layout/mod.rs b/crates/bevy_ui/src/layout/mod.rs
--- a/crates/bevy_ui/src/layout/mod.rs	(revision 8bbf91532c1fcdef9949ae021f3c3531c970c865)
+++ b/crates/bevy_ui/src/layout/mod.rs	(date 1711922950048)
@@ -63,7 +63,6 @@
     camera_entity_to_taffy: EntityHashMap<EntityHashMap<taffy::node::Node>>,
     camera_roots: EntityHashMap<Vec<RootNodePair>>,
     taffy: Taffy,
-    no_camera: bool,
 }
 
 fn _assert_send_sync_ui_surface_impl_safe() {
@@ -91,7 +90,6 @@
             camera_entity_to_taffy: Default::default(),
             camera_roots: Default::default(),
             taffy,
-            no_camera: false,
         }
     }
 }
@@ -340,7 +338,6 @@
                     );
                     continue;
                 };
-                ui_surface.no_camera = false;
                 let layout_info = camera_layout_info
                     .entry(camera_entity)
                     .or_insert_with(|| calculate_camera_layout_info(camera));
@@ -348,10 +345,7 @@
             }
             None => {
                 if cameras.is_empty() {
-                    if !ui_surface.no_camera {
-                        ui_surface.no_camera = true;
-                        warn!("No camera found to render UI to. To fix this, add at least one camera to the scene.");
-                    }
+                    warn!("No camera found to render UI to. To fix this, add at least one camera to the scene.");
                 } else {
                     warn!(
                         "Multiple cameras found, causing UI target ambiguity. \

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes perfect sense, I'll make the commit with that changes! Thank you for your help!

@james7132 james7132 added C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash labels Apr 2, 2024
@james7132 james7132 requested a review from rparrett April 2, 2024 03:04
min_size: 0.0,
max_size: 0.0,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This should be a Default trait impl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that make sense! Added those changes in the newest commit!

Copy link
Member

Choose a reason for hiding this comment

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

No, as in LayoutContext should implement Default instead of a default method like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

impl Default for LayoutContext {
   fn default() -> Self {
        Self {
            scale_factor: 1.0,
            physical_size: Vec2::ZERO,
            min_size: 0.0,
            max_size: 0.0,
        }
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I misunderstood your sugestion, thanks for clarifying!
The last commit should already have those changes implemented.

Copy link
Contributor

@StrikeForceZero StrikeForceZero left a comment

Choose a reason for hiding this comment

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

The merge of the main into the PR's branch makes me nervous but I think all PR's get squashed so it shouldn't matter. Everything else looks good.

@pablo-lua pablo-lua 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 Apr 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 22, 2024
Merged via the queue into bevyengine:main with commit 16ff354 Apr 22, 2024
1 check passed
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-Usability A targeted quality-of-life change that makes Bevy easier to use P-Crash A sudden unexpected crash 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.

Panic with UI hierarchy when no camera is present
6 participants