Why can't we round node width/height directly in roundLayoutResultsToPixelGrid()? #1574

New Issue

2024-02-07T14:11:39-08:00

kennethpu commented

2024-02-07 14:11:39 -08:00

(Migrated from github.com)

Report

I have searched existing issues and this is not a duplicate

In PixelGrid.roundLayoutResultsToPixelGrid() it appears that we are calculating a node's rounded width/height as follows:

Calculate the start position of the node.

const double absoluteNodeLeft = absoluteLeft + nodeLeft;

Using the start position and computed width, calculate the end position of the node.

const double absoluteNodeRight = absoluteNodeLeft + nodeWidth;

Calculate rounded node width/height as the difference between the rounded start/end positions of the node.

node->setLayoutDimension(
    roundValueToPixelGrid(
        absoluteNodeRight, 
        pointScaleFactor, 
        (textRounding && hasFractionalWidth), 
        (textRounding && !hasFractionalWidth)
    ) -
    roundValueToPixelGrid(
        absoluteNodeLeft, 
        pointScaleFactor, 
        false, 
        textRounding
     ),
     Dimension::Width);

It's not clear to me why we need to calculate rounded width/height in this way as opposed to rounding the width/height values directly, and it also seems like this may introduce rounding errors as noted in this previously opened issue (https://github.com/facebook/yoga/issues/901). Curious if the proposed solution on that issue has merit or if there are other issues not being considered.

As an iOS developer utilizing a fork of Yoga (really appreciate this project btw!) I'm also curious if you foresee any issues with tweaking the rounding logic on our fork to more closely mirror Apple's strategy of rounding down origin values and rounding up size values to the nearest pixel boundary. Are there other parts of the layout algorithm we would need to update for consistency?

# Report - [x] I have searched [existing issues](https://github.com/facebook/yoga/issues) and this is not a duplicate In [PixelGrid.roundLayoutResultsToPixelGrid()](https://github.com/facebook/yoga/blob/a37565f70d74ecc1a312a62dc48091fd2c73136e/yoga/algorithm/PixelGrid.cpp#L65C6-L65C35) it appears that we are calculating a node's rounded width/height as follows: 1. Calculate the start position of the node. ``` const double absoluteNodeLeft = absoluteLeft + nodeLeft; ``` 2. Using the start position and computed width, calculate the end position of the node. ``` const double absoluteNodeRight = absoluteNodeLeft + nodeWidth; ``` 3. Calculate rounded node width/height as the difference between the rounded start/end positions of the node. ``` node->setLayoutDimension( roundValueToPixelGrid( absoluteNodeRight, pointScaleFactor, (textRounding && hasFractionalWidth), (textRounding && !hasFractionalWidth) ) - roundValueToPixelGrid( absoluteNodeLeft, pointScaleFactor, false, textRounding ), Dimension::Width); ``` It's not clear to me why we need to calculate rounded width/height in this way as opposed to rounding the width/height values directly, and it also seems like this may introduce rounding errors as noted in this previously opened issue (https://github.com/facebook/yoga/issues/901). Curious if the [proposed solution](https://github.com/facebook/yoga/pull/902/commits/19035c90e9107226cc53d91f6722ee331339624a) on that issue has merit or if there are other issues not being considered. As an iOS developer utilizing a fork of Yoga (really appreciate this project btw!) I'm also curious if you foresee any issues with tweaking the rounding logic on our fork to more closely mirror [Apple's strategy](https://developer.apple.com/documentation/coregraphics/1456348-cgrectintegral) of rounding down origin values and rounding up size values to the nearest pixel boundary. Are there other parts of the layout algorithm we would need to update for consistency?

NickGerleman commented

2024-02-08 13:57:48 -08:00

(Migrated from github.com)

I would love to change how layout rounding works in terms of how it's executed, but haven't put a lot of thought into it's underlying rounding logic. We have seen real bugs from it.

A goal of this style of rounding is to not introduce "gaps", so it makes sense for the start position reference to be the previous rounded pixel I think.

The text rounding flag can on the other hand introduce overlaps, which is also wrong.

I would love to change how layout rounding works in terms of how it's executed, but haven't put a lot of thought into it's underlying rounding logic. We have seen real bugs from it. A goal of this style of rounding is to not introduce "gaps", so it makes sense for the start position reference to be the previous rounded pixel I think. The text rounding flag can on the other hand introduce overlaps, which is also wrong.

nicoburns commented

2024-02-11 13:18:18 -08:00

(Migrated from github.com)

Yoga's algorithm is indeed quite clever in it's avoidance of gaps in the resultant layout. The commit that introduced the current rounding mode to Yoga (aa5b296ac7) has an excellent writeup on the motivation. I believe Apple's strategy will result in 1px overlaps in some cases, which is perhaps not so bad (this definitely seems to me like a problem where there isn't really one best solution) but is something to bear in mind.

Yoga's algorithm is indeed quite clever in it's avoidance of gaps in the resultant layout. The commit that introduced the current rounding mode to Yoga (https://github.com/facebook/yoga/commit/aa5b296ac78f7a22e1aeaf4891243c6bb76488e2) has an excellent writeup on the motivation. I believe Apple's strategy will result in 1px overlaps in some cases, which is perhaps not so bad (this definitely seems to me like a problem where there isn't really one best solution) but is something to bear in mind.

nicoburns commented

2024-02-11 13:19:08 -08:00

(Migrated from github.com)

Regarding your having a fork of Yoga, are there also other changes that you have made locally? And would you be interested in upstreaming those?

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1574