Round pixel values of intrinsicSize with YGRoundPixelValue #1213

2023-01-14T19:53:30-08:00

stleamist commented

2023-01-14 19:53:30 -08:00

(Migrated from github.com)

This PR makes intrinsicSize return rounded pixel values using YGRoundPixelValue as YGApplyLayoutToViewHierarchy does.

This change prevents text truncation due to the low precision of C++ float when sizing the view containing UILabel using intinsicSize.

override func sizeToFit() {
  self.bounds.size = self.yoga.intrinsicSize
}

This PR makes `intrinsicSize` return rounded pixel values using `YGRoundPixelValue` as `YGApplyLayoutToViewHierarchy` does. This change prevents text truncation due to the low precision of C++ `float` when sizing the view containing `UILabel` using `intinsicSize`. ```swift override func sizeToFit() { self.bounds.size = self.yoga.intrinsicSize } ```

👍 1

NickGerleman commented

2023-01-15 20:54:20 -08:00

(Migrated from github.com)

Change makes sense to me.

For record-keeping, could you add a before/after screenshot to the PR description?

For awareness, the automated tests for YogaKit, and build on latest toolchain have been broken for a while, so we will have reduced coverage for this change against any existing automated tests.

Fixing that has been on the infrastructure todo list, but after fixing the CMake + GTest experience for C ++ UTs, and then UTs for JNI bindings/Android build. There are a lot of PRs fixing various bits of the Apple build, but I haven't had the experience with the ecosystem to understand the whole of what we should be doing to the existing build logic, vs if we should just axe it, follow the Cocoapods/SPM guide for building a new package + example, then follow the leads where it needs code-changes.

Change makes sense to me. For record-keeping, could you add a before/after screenshot to the PR description? For awareness, the automated tests for YogaKit, and build on latest toolchain have been broken for a while, so we will have reduced coverage for this change against any existing automated tests. Fixing that has been on the infrastructure todo list, but after fixing the CMake + GTest experience for C ++ UTs, and then UTs for JNI bindings/Android build. There are a lot of PRs fixing various bits of the Apple build, but I haven't had the experience with the ecosystem to understand the whole of what we should be doing to the existing build logic, vs if we should just axe it, follow the Cocoapods/SPM guide for building a new package + example, then follow the leads where it needs code-changes.

rozele commented

2023-01-20 18:13:31 -08:00

(Migrated from github.com)

Perhaps I'm missing something here, but as I understand, Yoga already returns values rounded to the pixel grid assuming you set the scale via YGConfigSetPointScaleFactor. Why do you need to round a value that should presumably already be rounded?

NickGerleman (Migrated from github.com) requested changes 2023-01-24 14:19:22 -08:00

NickGerleman (Migrated from github.com) left a comment

See comments from Eric, and request for test plan

NickGerleman commented

2023-02-07 20:39:42 -08:00

(Migrated from github.com)

Perhaps I'm missing something here, but as I understand, Yoga already returns values rounded to the pixel grid assuming you set the scale via YGConfigSetPointScaleFactor. Why do you need to round a value that should presumably already be rounded?

Just to check, is the idea here that:

Yoga already does its own rounding (YGRoundToPixelGrid()) based on pointScaleFactor, when laying out a node.
The YogaKit-specific YGRoundPixelValue() and its usage should be removed, and we should rely on Yoga to do all of the rounding itself

I am seeing that YogaKit is already calling YGConfigSetPointScaleFactor() (though assuming the density never changes, which is... probably fine on iOS). So this would end up looking like just removing the existing usage I think.

> Perhaps I'm missing something here, but as I understand, Yoga already returns values rounded to the pixel grid assuming you set the scale via YGConfigSetPointScaleFactor. Why do you need to round a value that should presumably already be rounded? Just to check, is the idea here that: 1. Yoga already does its own rounding (`YGRoundToPixelGrid()`) based on `pointScaleFactor`, when laying out a node. 2. The YogaKit-specific `YGRoundPixelValue()` and its usage should be removed, and we should rely on Yoga to do all of the rounding itself I am seeing that YogaKit is already calling `YGConfigSetPointScaleFactor()` (though assuming the density never changes, which is... probably fine on iOS). So this would end up looking like just removing the existing usage I think.

NickGerleman commented

2023-06-15 13:30:39 -07:00

(Migrated from github.com)

We are deprecating YogaKit as part of the Yoga 2.0 release. We are still going to release a new revision based on the current state of the repo, but won't be accepting new contributions, since we are going to be removing it from the repo after.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1213

Round pixel values of intrinsicSize with YGRoundPixelValue #1213

Pull request closed

Round pixel values of `intrinsicSize` with `YGRoundPixelValue` #1213