Mirrors/bevy
2
0
Fork 0
mirror of https://github.com/bevyengine/bevy synced 2024-11-21 20:23:28 +00:00
Code Issues Projects Releases Packages Wiki Activity

Remove custom rounding (#16097)

Browse source 
# Objective

Taffy added layout rounding a while ago but it had a couple of bugs and
caused some problems with the fussy `ab_glyph` text implementation. So I
disabled Taffy's builtin rounding and added some hacks ad hoc that fixed
(some) of those issues. Since then though Taffy's rounding algorithm has
improved while we've changed layout a lot and migrated to `cosmic-text`
so those hacks don't help any more and in some cases cause significant
problems.

Also our rounding implementation only rounds to the nearest logical
pixel, whereas Taffy rounds to the nearest physical pixel meaning it's
much more accurate with high dpi displays.

fixes #15197

## Some examples of layout rounding errors visible in the UI examples

These errors are much more obvious at high scale factor, you might not
see any problems at a scale factor of 1.

`cargo run --example text_wrap_debug`

<img width="1000" alt="text_debug_gaps"
src="https://github.com/user-attachments/assets/5a584016-b8e2-487b-8842-f0f359077391">

The narrow horizontal and vertical lines are gaps in the layout caused
by errors in the coordinate rounding.

`cargo run --example text_debug`

<img width="1000" alt="text_debug"
src="https://github.com/user-attachments/assets/a4b37c02-a2fd-441c-a7bd-cd7a1a72e7dd">

The two text blocks here are aligned right to the same boundary but in
this screen shot you can see that the lower block is one pixel off to
the left. Because the size of this text node changes between frames with
the reported framerate the rounding errors cause it to jump left and
right.

## Solution

Remove all our custom rounding hacks and reenable Taffy's layout
rounding.

The gaps in the `text_wrap_debug` example are gone:
<img width="1000" alt="text_wrap_debug_fix"
src="https://github.com/user-attachments/assets/92d2dd97-30c6-4ac8-99f1-6d65358995a7">

This doesn't fix some of the gaps that occur between borders and content
but they seem appear to be a rendering problem as they disappear with
`UiAntiAlias::Off` set.

## Testing

Run the examples as described above in the `Objective` section. With
this PR the problems mentioned shouldn't appear.

Also added an example in a separate PR #16096 `layout_rounding_debug`
for identifying these issues.

## Migration Guide

`UiSurface::get_layout` now also returns the final sizes before
rounding. Call `.0` on the `Ok` result to get the previously returned
`taffy::Layout` value.

---------

Co-authored-by: Rob Parrett <robparrett@gmail.com>
This commit is contained in:
ickshonpe 2024-10-28 22:20:50 +00:00 committed by GitHub
parent af279073aa
commit 05d90686fc
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 53 additions and 82 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

103
crates/bevy_ui/src/layout/mod.rs
View file

@ -296,14 +296,13 @@ with UI components as a child of an entity without UI components, your UI layout
update_uinode_geometry_recursive(
&mut commands,
*root,
&ui_surface,
&mut ui_surface,
None,
&mut node_transform_query,
&ui_children,
inverse_target_scale_factor,
Vec2::ZERO,
Vec2::ZERO,
Vec2::ZERO,
);
}
@ -315,7 +314,7 @@ with UI components as a child of an entity without UI components, your UI layout
fn update_uinode_geometry_recursive(
commands: &mut Commands,
entity: Entity,
ui_surface: &UiSurface,
ui_surface: &mut UiSurface,
root_size: Option<Vec2>,
node_transform_query: &mut Query<(
&mut ComputedNode,
@ -329,7 +328,6 @@ with UI components as a child of an entity without UI components, your UI layout
inverse_target_scale_factor: f32,
parent_size: Vec2,
parent_scroll_position: Vec2,
mut absolute_location: Vec2,
) {
if let Ok((
mut node,
@ -340,28 +338,24 @@ with UI components as a child of an entity without UI components, your UI layout
maybe_scroll_position,
)) = node_transform_query.get_mut(entity)
{
let Ok(layout) = ui_surface.get_layout(entity) else {
let Ok((layout, unrounded_unscaled_size)) = ui_surface.get_layout(entity) else {
return;
};
let layout_size =
inverse_target_scale_factor * Vec2::new(layout.size.width, layout.size.height);
let unrounded_size = inverse_target_scale_factor * unrounded_unscaled_size;
let layout_location =
inverse_target_scale_factor * Vec2::new(layout.location.x, layout.location.y);
absolute_location += layout_location;
let rounded_size = approx_round_layout_coords(absolute_location + layout_size)
- approx_round_layout_coords(absolute_location);
let rounded_location =
approx_round_layout_coords(layout_location - parent_scroll_position)
+ 0.5 * (rounded_size - parent_size);
// The position of the center of the node, stored in the node's transform
let node_center =
layout_location - parent_scroll_position + 0.5 * (layout_size - parent_size);
// only trigger change detection when the new values are different
if node.size != rounded_size || node.unrounded_size != layout_size {
node.size = rounded_size;
node.unrounded_size = layout_size;
if node.size != layout_size || node.unrounded_size != unrounded_size {
node.size = layout_size;
node.unrounded_size = unrounded_size;
}
let taffy_rect_to_border_rect = |rect: taffy::Rect<f32>| BorderRect {
@ -402,8 +396,8 @@ with UI components as a child of an entity without UI components, your UI layout
.max(0.);
}
if transform.translation.truncate() != rounded_location {
transform.translation = rounded_location.extend(0.);
if transform.translation.truncate() != node_center {
transform.translation = node_center.extend(0.);
}
let scroll_position: Vec2 = maybe_scroll_position
@ -423,11 +417,9 @@ with UI components as a child of an entity without UI components, your UI layout
})
.unwrap_or_default();
let round_content_size = approx_round_layout_coords(
Vec2::new(layout.content_size.width, layout.content_size.height)
* inverse_target_scale_factor,
);
let max_possible_offset = (round_content_size - rounded_size).max(Vec2::ZERO);
let content_size = Vec2::new(layout.content_size.width, layout.content_size.height)
* inverse_target_scale_factor;
let max_possible_offset = (content_size - layout_size).max(Vec2::ZERO);
let clamped_scroll_position = scroll_position.clamp(Vec2::ZERO, max_possible_offset);
if clamped_scroll_position != scroll_position {
@ -445,36 +437,14 @@ with UI components as a child of an entity without UI components, your UI layout
node_transform_query,
ui_children,
inverse_target_scale_factor,
rounded_size,
layout_size,
clamped_scroll_position,
absolute_location,
);
}
}
}
}
#[inline]
/// Round `value` to the nearest whole integer, with ties (values with a fractional part equal to 0.5) rounded towards positive infinity.
fn approx_round_ties_up(value: f32) -> f32 {
(value + 0.5).floor()
}
#[inline]
/// Rounds layout coordinates by rounding ties upwards.
///
/// Rounding ties up avoids gaining a pixel when rounding bounds that span from negative to positive.
///
/// Example: The width between bounds of -50.5 and 49.5 before rounding is 100, using:
/// - `f32::round`: width becomes 101 (rounds to -51 and 50).
/// - `round_ties_up`: width is 100 (rounds to -50 and 50).
fn approx_round_layout_coords(value: Vec2) -> Vec2 {
Vec2 {
x: approx_round_ties_up(value.x),
y: approx_round_ties_up(value.y),
}
}
#[cfg(test)]
mod tests {
use taffy::TraversePartialTree;
@ -493,7 +463,7 @@ mod tests {
use bevy_hierarchy::{
despawn_with_children_recursive, BuildChildren, ChildBuild, Children, Parent,
};
use bevy_math::{vec2, Rect, UVec2, Vec2};
use bevy_math::{Rect, UVec2, Vec2};
use bevy_render::{
camera::{ManualTextureViews, OrthographicProjection},
prelude::Camera,
@ -510,21 +480,10 @@ mod tests {
};
use crate::{
layout::{approx_round_layout_coords, ui_surface::UiSurface},
prelude::*,
ui_layout_system,
update::update_target_camera_system,
ContentSize, LayoutContext,
layout::ui_surface::UiSurface, prelude::*, ui_layout_system,
update::update_target_camera_system, ContentSize, LayoutContext,
};
#[test]
fn round_layout_coords_must_round_ties_up() {
assert_eq!(
approx_round_layout_coords(vec2(-50.5, 49.5)),
vec2(-50., 50.)
);
}
// these window dimensions are easy to convert to and from percentage values
const WINDOW_WIDTH: f32 = 1000.;
const WINDOW_HEIGHT: f32 = 100.;
@ -598,10 +557,10 @@ mod tests {
world.entity_mut(ui_root).add_child(ui_child);
ui_schedule.run(&mut world);
let ui_surface = world.resource::<UiSurface>();
let mut ui_surface = world.resource_mut::<UiSurface>();
for ui_entity in [ui_root, ui_child] {
let layout = ui_surface.get_layout(ui_entity).unwrap();
let layout = ui_surface.get_layout(ui_entity).unwrap().0;
assert_eq!(layout.size.width, WINDOW_WIDTH);
assert_eq!(layout.size.height, WINDOW_HEIGHT);
}
@ -955,11 +914,12 @@ mod tests {
.get_single(world)
.expect("missing MovingUiNode");
assert_eq!(expected_camera_entity, target_camera_entity);
let ui_surface = world.resource::<UiSurface>();
let mut ui_surface = world.resource_mut::<UiSurface>();
let layout = ui_surface
.get_layout(ui_node_entity)
.expect("failed to get layout");
.expect("failed to get layout")
.0;
// negative test for #12255
assert_eq!(Vec2::new(layout.location.x, layout.location.y), new_pos);
@ -1043,8 +1003,8 @@ mod tests {
ui_schedule.run(&mut world);
let ui_surface = world.resource::<UiSurface>();
let layout = ui_surface.get_layout(ui_entity).unwrap();
let mut ui_surface = world.resource_mut::<UiSurface>();
let layout = ui_surface.get_layout(ui_entity).unwrap().0;
// the node should takes its size from the fixed size measure func
assert_eq!(layout.size.width, content_size.x);
@ -1068,12 +1028,12 @@ mod tests {
ui_schedule.run(&mut world);
let ui_surface = world.resource::<UiSurface>();
let mut ui_surface = world.resource_mut::<UiSurface>();
let ui_node = ui_surface.entity_to_taffy[&ui_entity];
// a node with a content size should have taffy context
assert!(ui_surface.taffy.get_node_context(ui_node).is_some());
let layout = ui_surface.get_layout(ui_entity).unwrap();
let layout = ui_surface.get_layout(ui_entity).unwrap().0;
assert_eq!(layout.size.width, content_size.x);
assert_eq!(layout.size.height, content_size.y);
@ -1081,12 +1041,12 @@ mod tests {
ui_schedule.run(&mut world);
let ui_surface = world.resource::<UiSurface>();
let mut ui_surface = world.resource_mut::<UiSurface>();
// a node without a content size should not have taffy context
assert!(ui_surface.taffy.get_node_context(ui_node).is_none());
// Without a content size, the node has no width or height constraints so the length of both dimensions is 0.
let layout = ui_surface.get_layout(ui_entity).unwrap();
let layout = ui_surface.get_layout(ui_entity).unwrap().0;
assert_eq!(layout.size.width, 0.);
assert_eq!(layout.size.height, 0.);
}
@ -1123,7 +1083,8 @@ mod tests {
.collect::<Vec<Entity>>();
for r in [2, 3, 5, 7, 11, 13, 17, 19, 21, 23, 29, 31].map(|n| (n as f32).recip()) {
let mut s = r;
// This fails with very small / unrealistic scale values
let mut s = 1. - r;
while s <= 5. {
world.resource_mut::<UiScale>().0 = s;
ui_schedule.run(&mut world);

32
crates/bevy_ui/src/layout/ui_surface.rs
View file

@ -6,7 +6,7 @@ use bevy_ecs::{
entity::{Entity, EntityHashMap},
prelude::Resource,
};
use bevy_math::UVec2;
use bevy_math::{UVec2, Vec2};
use bevy_utils::default;
use crate::{layout::convert, LayoutContext, LayoutError, Measure, MeasureArgs, Node, NodeMeasure};
@ -51,8 +51,7 @@ impl fmt::Debug for UiSurface {
impl Default for UiSurface {
fn default() -> Self {
let mut taffy: TaffyTree<NodeMeasure> = TaffyTree::new();
taffy.disable_rounding();
let taffy: TaffyTree<NodeMeasure> = TaffyTree::new();
Self {
entity_to_taffy: Default::default(),
camera_entity_to_taffy: Default::default(),
@ -276,14 +275,25 @@ impl UiSurface {
/// Get the layout geometry for the taffy node corresponding to the ui node [`Entity`].
/// Does not compute the layout geometry, `compute_window_layouts` should be run before using this function.
pub fn get_layout(&self, entity: Entity) -> Result<&taffy::Layout, LayoutError> {
if let Some(taffy_node) = self.entity_to_taffy.get(&entity) {
self.taffy
.layout(*taffy_node)
.map_err(LayoutError::TaffyError)
} else {
Err(LayoutError::InvalidHierarchy)
}
/// On success returns a pair consisiting of the final resolved layout values after rounding
/// and the size of the node after layout resolution but before rounding.
pub fn get_layout(&mut self, entity: Entity) -> Result<(taffy::Layout, Vec2), LayoutError> {
let Some(taffy_node) = self.entity_to_taffy.get(&entity) else {
return Err(LayoutError::InvalidHierarchy);
};
let layout = self
.taffy
.layout(*taffy_node)
.cloned()
.map_err(LayoutError::TaffyError)?;
self.taffy.disable_rounding();
let taffy_size = self.taffy.layout(*taffy_node).unwrap().size;
let unrounded_size = Vec2::new(taffy_size.width, taffy_size.height);
self.taffy.enable_rounding();
Ok((layout, unrounded_size))
}
}