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

Add resize mode cover/contain #644

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

lichstam
Copy link
Contributor

@lichstam lichstam commented Feb 5, 2024

Motivation

I've added support for a dynamic resizeMode in the RealCamera class to address the need for flexible aspect ratio handling of the camera's preview layer. This enhancement allows switching between .resizeAspectFill and .resizeAspect, improving UI flexibility. My main goal was to enable both a full-cover and contained fit for the camera preview without manual adjustments. This feature is critical for applications requiring dynamic adjustments to the camera preview based on user interaction or specific UI layouts.

Testing on iOS

Despite not being a Swift developer, I managed to implement and test these changes on iOS:

Manual Testing: Conducted to ensure the resizeMode property correctly updates the videoGravity of the AVCaptureVideoPreviewLayer.
UI Verification: Confirmed the UI updates accordingly, without any disruptions or visual glitches.
Regression Testing: Checked that existing functionalities (camera initialization, capture functionality, zoom, and focus adjustments) remain unaffected.
Testing was limited to iOS due to my current setup. Android testing is needed to ensure cross-platform compatibility and functionality.

@@ -105,6 +105,19 @@ public enum ZoomMode: Int, CustomStringConvertible {
}
}

@objc(CKResizeMode)
public enum ResizeMode: Int {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make it consistent with the other ones, like the one above?

No = 0, implements CustomStringConvertible, description instead of stringValue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lichstam what about this one?

Copy link
Contributor

@DavidBertet DavidBertet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lichstam!

Can you:

  • add the documentation in the README
  • check CameraProps where I guess it should be added in iOS specific section
  • see if we can add it in the example somehow? so we can easily test/showcase it
  • maybe add a label in the simulator to see what mode we are on

Who is calling func update(resizeMode: ResizeMode)? I feel like the logic in CameraView is missing, but you seem to say that its working

@lichstam
Copy link
Contributor Author

i'll have a look tomorrow!

@lichstam lichstam force-pushed the feature/resize-mode branch from 355e711 to 5f4c07a Compare February 27, 2024 19:50
@lichstam lichstam force-pushed the feature/resize-mode branch from 5f4c07a to 8978035 Compare February 27, 2024 19:50
@lichstam
Copy link
Contributor Author

RPReplay_Final1709062570.MP4

This is how I made the example

All other issues are addressed. You were right btw, somehow the call to the function didn't make it in the first iteration of this PR :)

Copy link
Contributor

@DavidBertet DavidBertet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On small feedback, but otherwise it's looking good 👍

@@ -27,6 +27,7 @@ class RealCamera: NSObject, CameraProtocol, AVCaptureMetadataOutputObjectsDelega
private let photoOutput = AVCapturePhotoOutput()
private let metadataOutput = AVCaptureMetadataOutput()

private var resizeMode: ResizeMode = .contain
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we do .contain here, and .cover in CameraView? Seems like we should have contain for all default values to keep things consistent (and say so in the doc)

@lichstam
Copy link
Contributor Author

i'll take a look at the minor comments, however, is this repo even alive?

ios/ReactNativeCameraKit/CameraView.swift Outdated Show resolved Hide resolved
@scarlac scarlac merged commit 80c517d into teslamotors:master Apr 30, 2024
3 of 4 checks passed
@scarlac
Copy link
Collaborator

scarlac commented Apr 30, 2024

@lichstam Yes, still alive. Thank you for the contribution for iOS.

@lichstam
Copy link
Contributor Author

np :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants