Repository navigation
fix(ios): correct ratioOverlay aspect calculation and orientation - #818
Open
ZayanKhan-12 wants to merge 1 commit into
Open
ZayanKhan-12 wants to merge 1 commit into
ZayanKhan-12 wants to merge 1 commit into
Conversation
`ratioOverlay` produced a window that was both inverted and the wrong
shape for every ratio except `1:1`.
Two separate defects:
1. Parsing was reversed. `RatioOverlayData` read `values[0]` into
`height` and `values[1]` into `width`, so `'4:5'` was treated as a
5:4 window. The README documents the prop as `'16:9'`, i.e.
width:height, and that is what callers expect.
2. The geometry was wrong. The centre window was derived from the
opposite axis of the preview: `centerSize.height` was
`frame.height * ratio` (and `centerSize.width` was
`frame.width / ratio`) instead of being derived from the axis being
held fixed. The result depended on the preview's own aspect ratio, so
the window never actually matched the requested ratio. `1:1` was
hard-coded on a separate branch, which is why only `1:1` looked right.
On a 375x812 preview, master produced:
input window actual ratio bars
1:1 375x375 1.000 top/bottom ok
4:5 300x812 0.369 left/right wrong
5:4 375x650 0.577 top/bottom wrong
16:9 375x457 0.821 top/bottom wrong
The ratio window is now computed as a plain aspect fit: the largest rect
of the requested ratio that fits the preview, centred, with the leftover
strips masked. That collapses the three near-duplicate branches into one
and makes `1:1` fall out of the general case rather than being special.
Also guards against zero/negative components and zero-sized previews,
which previously produced a NaN or an inverted frame instead of simply
hiding the overlay.
Fixes teslamotors#195
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #195
The bug
ratioOverlayrenders a window that is both inverted and the wrong shape for every ratio except1:1— exactly as reported in #195, where'4:5'drew vertical bars and'5:4'drew a window that was neither 4:5 nor 5:4.There are two independent defects in
RatioOverlayView.swift.1. The ratio string is parsed backwards.
RatioOverlayData.init(from:)readvalues[0]intoheightandvalues[1]intowidth:So
'4:5'was handled as a 5:4 window. The README documents the prop with'16:9', i.e.width:height, and that is what callers expect.2. The geometry derives the window from the wrong axis. The centre window was sized as
frame.height * ratio(orframe.width / ratio) instead of from the axis actually being held fixed. The output therefore depended on the preview's aspect ratio rather than only on the requested one, so the window never matched the ratio asked for.1:1sat on its own hard-coded branch, which is why1:1was the only case users saw working.Measured on a 375×812 preview,
masterproduces:1:14:55:416:99:164:3The fix
Parse
width:height, then compute the window as a plain aspect fit: the largest rect of the requested ratio that fits inside the preview, centred, with the leftover strips masked.This collapses the three near-duplicate branches into one, and
1:1now falls out of the general case instead of needing its own branch. Theswiftlint:disable function_body_lengthsuppression is no longer needed and has been removed.It also guards against zero/negative components (
'4:0','-4:5') and a zero-sized preview, which previously producedNaNor an inverted frame rather than simply hiding the overlay.Verification
The layout math was extracted from the shipped file and exercised directly — 343 assertions, all passing:
width:height, and 12 malformed inputs ('4:0','-4:5','4:5:6','x:y', …) hide the overlay instead of drawing garbage;style, iPad), the window always (a) has the requested ratio, (b) fits inside the preview, (c) touches at least one edge — i.e. it is maximal, and (d) is centred.The real
RatioOverlayViewwas then laid out by UIKit, rendered to a bitmap, and the visible window measured back out of the pixels:'4:5'now draws a window taller than it is wide with bars top and bottom — the behaviour the issue asks for.Also run locally:
swiftlinton the changed file (0 violations),yarn build,yarn lint, and aReactNativeCameraKitpod-target build foriphoneos/arm64 against the example app's CocoaPodssetup with Fabric codegen —
** BUILD SUCCEEDED **. (The fullCameraKitExamplescheme does notbuild on Xcode 26: the vendored
fmtpod fails withcall to consteval function ... is not a constant expression, a React Native dependency issue unrelated to this change.)Compatibility
This changes observable behaviour for anyone who compensated for the bug by passing an inverted string (e.g.
'5:4'to get a 4:5 window). That is unavoidable if the prop is to match its documentation — the README has been updated to state thewidth:heightconvention explicitly. Worth calling out in release notes.