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

Add configurable options to improve output.screen node #547

Merged
merged 5 commits into from
Dec 17, 2021
Merged

Add configurable options to improve output.screen node #547

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
901b4a7
feat: add configs to output screen
leeping-ng Dec 1, 2021
9fe2def
feat: nested related configs for cleaner code
leeping-ng Dec 2, 2021
aafeb28
feat: update resizing on opencv version >= 4.5
leeping-ng Dec 16, 2021
98d3f78
feat: minor refactor to streamline code
leeping-ng Dec 17, 2021
9762dc8
feat: added note in config that window size can be changed by clickin…
leeping-ng Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion peekingduck/configs/output/screen.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
input: ["img"]
output: ["pipeline_end"]
output: ["pipeline_end"]

window_name: PeekingDuck
window_width: 1280
window_height: 720
window_x_coord: 0
window_y_coord: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest OOP-ing the above with a window object:

window: {
    name: "PeekingDuck",
    x: 0,
    y: 0,
    width: 1280,
    height: 720,
}

Facilitates future attribute extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of this at first, but thought it would be clunky when users read our RTD page. I'd have to lump all the descriptions under one Dict type which can look messy with different descriptions. Also there's a mix of int and str types that get lost when all are lumped under Dict.

Example from resize config from input.live - I think this is simple enough to be nested though.
image

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could do it this way, which is a good compromise:

window_name: "PeekingDuck"
window_loc: {
   x: 0,
   y: 0 }
window_size: {
   width: 1280,
   height: 720 }

Copy link
Contributor

@ongtw ongtw Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. The resize (Dict) example also relies on the user to infer the bool and int types, so it actually servers to highlight the point made on lost type info.
  2. The alternative you suggested is a compromise that I am neutral about.
  3. Another suggestion is to remove window_ totally, just have
inputs: [...]
outputs: [...]

name: "PeekingDuck"
x: 0
y: 0
width: 1280
height: 720

and the code will be self.height, self.width, etc.
Since, after all, the screen is the display.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried the last suggestion, but the keyword name causes an AttributeError: can't set attribute.

In the end I went for the in-between solution, the RTD looks like this:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, let's go with the in-between solution.
Good reminder that name is reserved, for future similar cases, we'll have to use title or label.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resizeWindow() doesn't work for opencv >= 4.5.x, only applying resize() to the image works. Unfortunately, it is the opposite case for opencv == 4.1.2.30. To avoid discrepancies in behaviour, in this PR, will go with the solution that works for 4.5.x, and will create another PR to upgrade our openCV version (issue #556).

Added a boolean config for resizing window that's consistent with input.live and input.recorded.

20 changes: 18 additions & 2 deletions peekingduck/pipeline/nodes/output/screen.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,33 @@ class Node(AbstractNode):
|pipeline_end|

Configs:
None.
window_name (:obj:`str`): **default = "PeekingDuck"** |br|
Name of the displayed window.
window_width (:obj:`int`): **default = 1280** |br|
Width of the displayed window, in pixels.
window_height (:obj:`int`): **default = 720** |br|
Height of the displayed window, in pixels.
window_x_coord (:obj:`int`): **default = 0** |br|
X-coordinate of the top left corner of the displayed window, with reference
from the top left corner of the screen, in pixels.
window_y_coord (:obj:`int`): **default = 0** |br|
Y-coordinate of the top left corner of the displayed window, with reference
from the top left corner of the screen, in pixels.
"""

def __init__(self, config: Dict[str, Any] = None, **kwargs: Any) -> None:
super().__init__(config, node_path=__name__, **kwargs)
cv2.namedWindow(self.window_name, cv2.WINDOW_NORMAL)
cv2.moveWindow(self.window_name, self.window_x_coord, self.window_y_coord)
cv2.resizeWindow(self.window_name, self.window_width, self.window_height)
Copy link
Contributor

Choose a reason for hiding this comment

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

In line with the suggested window object above, Node configs will accept a window object and code will reference its attributes via self.window.name, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated accordingly.


def run(self, inputs: Dict[str, Any]) -> Dict[str, Any]:
"""Show the outputs on your display"""
cv2.imshow("PeekingDuck", inputs["img"])

cv2.imshow(self.window_name, inputs["img"])

if cv2.waitKey(1) & 0xFF == ord("q"):
cv2.destroyWindow(self.window_name)
return {"pipeline_end": True}

return {"pipeline_end": False}
ongtw marked this conversation as resolved.
Show resolved Hide resolved