-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Adding some features to openCV / Touchscreen #530
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this pull request, I haven't tested but it seems very promising for the opencv part. I have though some comments mainly to be sure that the code is not getting unnecessary parameters. I hope we can find way to merge it.
# Use V4L2 instead of GSTREAMER | ||
use_v4l2 = False | ||
|
||
# Custom FourCC (can be YUY2 or MJPG or any other fourcc or None to use the default) | ||
custom_fourcc = None | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very fan to add opencv only settings in the config (I don't think we have this right now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you suggest ? Perhaps create a specific bloc in configuration for opencv [OPENCV] in the ini file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in another comment, at least put more info that these parameters are only used for opencv camera
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already finished a refactor with a dedicated [OPENCV] configuration section. ^^
pibooth/camera/base.py
Outdated
self.delete_internal_memory = False | ||
self.preview_rotation, self.capture_rotation = (0, 0) | ||
self.preview_iso, self.capture_iso = (100, 100) | ||
self.preview_flip, self.capture_flip = (False, False) | ||
self.custom_fourcc = "Default" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same kind of comment: the fourcc
part shall not be in the base
camera
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right, i will try to move it to OpenCV class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I want to mode some opencv specific configuration values, i think i will need to refactor the configuration of the cameras. Currentyl, some "general" parameters like iso, resolution (delete_internal_memory <== Specific paramter) are passed directly) to the initialize method. But all the cameras does not share the same parameters. IMHO it will be better to passe the cfg to each derived class and each class will retrieve the custom parameters according to its need . How do you think about this solution ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, you are right we already have exception and it needs a bigger change to work differently. I think the best is to avoid making a bigger change in this PR and keep it like it, maybe just be more explicit on the parameters name like opencv_use_v4l2
and opencv_custom_fourcc
or at least be explicit in the config.py file that these parameters are used only for opencv cameras
@@ -248,7 +248,7 @@ def show_intro(self, pil_image=None, with_print=True): | |||
else: | |||
self._update_background(background.IntroBackground(self.arrow_location, self.arrow_offset)) | |||
|
|||
if pil_image: | |||
if with_print and pil_image: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding this criteria?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this test for the fullscreen intro screen, without it, the text and the previous image will overlap together
pibooth/view/background.py
Outdated
@@ -498,7 +501,7 @@ def resize(self, screen): | |||
# Right arrow | |||
self.right_arrow = pictures.get_pygame_image( | |||
"printer_touch.png", size, hflip=False, vflip=False, color=self._text_color) | |||
x = int(self._rect.left + self._rect.width * 0.70 | |||
x = int(self._rect.left + self._rect.width * 0.80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why changing this ratio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the ratio because the text is too much positioned on the left side. I've made some tests and the users touch the text and in this case, it trigger a print instead of forgotting the photo. But even with this change, the text positionning for touchscreen are not really good. I will perhaps made some refactor of text positionning for the touchscreen version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we cannot move the text to the left as the picture in portrait mode is taking the full height, that's the main reason we kept it on the right. The best workaround we gave was to update the text to be more explicit like "Appuyer sur la photo pour l'oublier" but we cannot make it default as the pibooth can be used without a touchscreen and in this case the arrow is showing which button should be pressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this, with the text below the left image and the printer centered
And in portrait mode, the image is slowly reduced to show the text below (only in touchscreen mode)
I've added a test if we are in portrait mode and touchscreen mode, in this case I align on top and reduce to 80% the image. What do you think about this solution ?
@@ -195,22 +196,24 @@ def paint(self, screen): | |||
|
|||
class IntroBackground(Background): | |||
|
|||
def __init__(self, arrow_location=ARROW_BOTTOM, arrow_offset=0): | |||
def __init__(self, arrow_location=ARROW_BOTTOM, arrow_offset=0, full_width=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full_width option is only used for arrow_location=ARROW_TOUCH and no print available, I guess we can find a way to avoid using a new param for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I will try to remove the parameter
pibooth/camera/opencv.py
Outdated
self._cam.set(cv2.CAP_PROP_ISO_SPEED, self.preview_iso) | ||
if self.custom_fourcc != "Default": | ||
LOGGER.info("Using custon fourcc %s", self.custom_fourcc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOGGER.info("Using custon fourcc %s", self.custom_fourcc) | |
LOGGER.info("Using custom fourcc %s", self.custom_fourcc) |
Added some usefull features to opencv when using a Webcam :
For touchscreen, allow a fullwidth intro screen