-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix panic on Text UI without Cameras #11405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple changes that will probably fix the bug.
If we go about improving warnings, there are some points that I suggest a change (Maybe in a future PR):
None => {
if cameras.is_empty() {
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. \
To fix this, add an explicit `TargetCamera` component to the root UI node {:?}",
entity
);
}
continue;
}
In this part, we warn the user about the existence of "multiple" cameras, but this warn isn't exactly true. In fact, this function return None
only if there is no cameras pointing to the PrimaryWindow
. If there is no Cameras pointing to the PrimaryWindow
and cameras isn't empty, the warn message isn't exactly accurate.
One possible messages is: "No camera found for PrimaryWindow, but found Cameras for other windows"
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
&mut self, | ||
entity: Entity, | ||
measure_func: taffy::node::MeasureFunc, | ||
) -> Option<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should return a Result
, not an Option
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do that in a future PR.
@@ -305,7 +310,7 @@ pub fn ui_layout_system( | |||
Some(camera_entity) => { | |||
let Ok((_, camera)) = cameras.get(camera_entity) else { | |||
warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should debounce this warning using the macro from #10808 so we don't spam the users :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider that but ended up not doing that because different entities could trigger that warning, but it will only trigger for the first entity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's leave it as is for now I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to clean it up further (like a result type), but I won't block on that. Fixing the crash is more important.
Why is this marked as a breaking change? |
|
It's in a private module. |
We should remove its |
Objective
Fix #11396.
Solution
Don't panic on taffy node not existing.
Plus minor warning text improvement.