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.

Sign up for GitHub

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

Jump to bottom

Added some missing options #44

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Added some missing options #44

wants to merge 10 commits into from

Conversation

GastonZalba
Copy link

@GastonZalba GastonZalba commented Jan 25, 2023 •
edited
Loading

Shared options

  • videoStabilization
  • colorEffect
  • dynamicRange
  • imageEffectMode
  • statistics
  • meteringMode
  • flickerMode
  • roi
  • awbGains
  • annotate
  • annotateExtra

Still options

  • raw
  • quality
  • thumbnail
  • burst
  • exif
  • gpsExif

Added HQ Camera mode

gregnr
gregnr suggested changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@gregnr gregnr left a comment

Choose a reason for hiding this comment

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

This is really great @GastonZalba! Thanks for your work on this, and sorry for the delay. I just a few DX things I would love to get your thoughts on before merging.

src/lib/shared-args.ts Outdated Show resolved Hide resolved
src/lib/shared-args.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@GastonZalba
Copy link
Author

Hi, excelent. I'm going to look at this and come back.

Another thing that would seem appropriate to do (i can make a separate PR) would be to move all the enums to a separate file and then reexport them from index.ts. So, the current behaviour would be the same, but it will allow to import only the enums (using something like pi-camera-connect/dist/@enums) from a frontend appplication (like react/electron) without requiring the entire library and preventing the fatal import errors from child_process and stream dependencies.

Sorry if my english is a bit broken.

@GastonZalba
Copy link
Author

@gregnr changes applied. Let me know what you think about the previous enums suggestion (move them to a separate file).
I'm also working on enabling the preview window and arguments (fullscreen, preview, keypress and dispnum). I'll be back in a few days with that.

gregnr
gregnr reviewed Feb 23, 2023
View reviewed changes
src/lib/shared-args.ts Outdated Show resolved Hide resolved
@GastonZalba GastonZalba marked this pull request as draft February 25, 2023 18:52
@GastonZalba GastonZalba marked this pull request as ready for review February 26, 2023 22:06
@GastonZalba
Copy link
Author

@gregnr Hi, i finally added a few more options (annotate, exif and related), expanded a little the still test, and cleaned up some mess that i left after the last requested changes. In the README I also modified some values that I had set as "default" from null to undefined, which I understand is more correct (and added some missing info as well). Let me know if any further adjustment is needed

@GastonZalba GastonZalba requested a review from gregnr February 26, 2023 22:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregnr gregnr Awaiting requested review from gregnr

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GastonZalba @gregnr