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

Rounding errors can accumulate and cause nodes to expand each time a layout is recomputed #501

Closed
ickshonpe opened this issue Jun 21, 2023 · 3 comments · Fixed by #521
Closed
Labels
bug Something isn't working
Milestone

Comments

@ickshonpe
Copy link
Contributor

taffy version

0.3.10 and main 868b85d

What you did

use taffy::prelude::*;
use taffy::util::print_tree;

fn main() {
    let mut taffy = Taffy::new();

    let inner = taffy.new_leaf(Style { min_size: Size { width: length(300.), height: auto() }, ..Default::default() }).unwrap();
    let wrapper = taffy.new_with_children( 
        Style {
            size: Size { width: length(150.), height: auto() },
            justify_content: Some(JustifyContent::End),
            ..Default::default()
        }, 
        &[inner]
    ).unwrap();
    let outer = taffy.new_with_children( 
        Style {
            size: Size { width: percent(100.), height: auto() },            
            inset: Rect { left: length(1.5), right: auto(), top: auto(), bottom: auto() },
            ..Default::default()
        },
        &[wrapper]
    ).unwrap();
    let viewport =  taffy.new_with_children(
        Style { 
            size: Size { width: length(1920.),height: length(1080.) },
            ..Default::default()
        },
        &[outer]
    ).unwrap();
    for _ in 0..3 {
        taffy.mark_dirty(viewport).ok();
        taffy.compute_layout(viewport, Size::MAX_CONTENT).ok();
        print_tree(&taffy, viewport);
        println!();
    }
}

What went wrong

TREE
└──  FLEX [x: 0    y: 0    width: 1920 height: 1080] (NodeId(4294967300))
    └──  FLEX [x: 2    y: 0    width: 1920 height: 1080] (NodeId(4294967299))
        └──  FLEX [x: 0    y: 0    width: 150  height: 1080] (NodeId(4294967298))
            └──  LEAF [x: -150 y: 0    width: 301  height: 1080] (NodeId(4294967297))

TREE
└──  FLEX [x: 0    y: 0    width: 1920 height: 1080] (NodeId(4294967300))
    └──  FLEX [x: 2    y: 0    width: 1920 height: 1080] (NodeId(4294967299))
        └──  FLEX [x: 0    y: 0    width: 150  height: 1080] (NodeId(4294967298))
            └──  LEAF [x: -150 y: 0    width: 302  height: 1080] (NodeId(4294967297))

TREE
└──  FLEX [x: 0    y: 0    width: 1920 height: 1080] (NodeId(4294967300))
    └──  FLEX [x: 2    y: 0    width: 1920 height: 1080] (NodeId(4294967299))
        └──  FLEX [x: 0    y: 0    width: 150  height: 1080] (NodeId(4294967298))
            └──  LEAF [x: -150 y: 0    width: 303  height: 1080] (NodeId(4294967297))

The width of the inner node expands by a pixel each time the layout is recomputed.

Additional information

This only happens when rounding is enabled.

@ickshonpe
Copy link
Contributor Author

ickshonpe commented Jun 21, 2023

The problem seems to be how rust rounds ties away from zero. So here the node ranges horizontally from -148.5 to 151.5 and then after rounding away from zero the range becomes -149 to 152, increasing the width by 1.

Replacing the round function in round_layout with

fn round_half_up(
    value: f32,
) -> f32 {
    if 0. < value ||  value.trunc() - value != 0.5 {
        value.round()
    } else {
        value.ceil()
    }
}

seems to work, but I'm not sure about it.

@nicoburns
Copy link
Collaborator

Huh. This is bizarre. Relayouts should not in any way depend on rounded values from previous layout runs. I'm thinking this is really a caching issue (using stale rounded values) rather than a rounding issue. I think we might need to:

  • Persist unrounded values between layout runs (we ought to already have this in the cache, so possibly we can grab them from there).
  • Perform rounding on every node each time (or have smarter cache invalidation for recomputing rounding).

But that's just a guess at this point. I will need to investigate properly.

@nicoburns
Copy link
Collaborator

Ah, so layout doesn't take rounded values as an input. But the rounding process itself does. I think the fix for this will indeed be to store unrounded values in addition to rounded values. We could also look at changing the rounding algorithm to round_ties_up, but I think that decision can be made purely on the basis of desired rounding behaviour rather than as a bug fix.

@nicoburns nicoburns added this to the 0.3.13 milestone Jul 17, 2023
nicoburns added a commit that referenced this issue Jul 23, 2023
* WIP

* When rounding is enabled, also store unrounded layout values.

Fixes #501

* Remove get_raw method from Cache struct

* Update rounding relayout test to actually assert

* cargo fmt

* Fix doc links

* Rename abs_x/abs_y to cumulative_x/cumulative_y

* Document when final_layout is rounded vs unrounded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants