Allow zones to be edited #186
Conversation
|
Wouldn't it be just as simple to have the Zone coords be reset, instead of adding lines? I don't think this PR is useful right now at all, unless it lets you start a zone from scratch. |
| # TODO(Bryce Beagle): Do this dynamically: | ||
| # TODO(Bryce Beagle): Do this dynamically: |
There was a problem hiding this comment.
| # TODO(Bryce Beagle): Do this dynamically: | |
| # TODO(Bryce Beagle): Do this dynamically: | |
| # TODO(Bryce Beagle): Do this dynamically: |
dedup
| @@ -0,0 +1,136 @@ | |||
| from typing import List, Optional | |||
There was a problem hiding this comment.
Is this all new, or from another file?
There was a problem hiding this comment.
A bit of both. The file was moved and changed, and that confused git enough
There was a problem hiding this comment.
Which file was it from? I can manually diff it on my end to try to provide feedback
There was a problem hiding this comment.
brainframe_qt/ui/dialogs/task_configuration/video_task_config/video_task_config.py
brainframe_qt/ui/resources/ui_elements/applications/singleton_application.py
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,154 @@ | |||
| from abc import ABC, abstractmethod | |||
| from dataclasses import dataclass, field | |||
There was a problem hiding this comment.
I believe that previously, the Screen was always first in the list. I think if it's no big deal to add, it might be nice to keep that 'feature'.
There was a problem hiding this comment.
Umm weird... I didn't think I changed anything that would affect ordering here. I also haven't seen Screen anywhere but the top. I'll investigate
There was a problem hiding this comment.
So I think the only way this could happen is if api.get_zones returns Screen not as the first Zone. Do you know why that would happen?
There was a problem hiding this comment.
I tested it, and it appears that api.get_zones() returns Screen first, but api.get_zones(stream_id=...) returns Screen last (or maybe everything just in inverse order?).
I only have two streams of sample size, but hey.
Since this is more of a UI thing, we probably don't want to rely on the server to maintain this visual nicety. We could add this after the api.get_zones() call, to ensure the Screen zone is always first:
zones = list(map(Zone.from_api_zone, api_zones))
# These lines
screen_index = next(i for i, z in enumerate(zones) if z.name == FULL_FRAME_ZONE_NAME)
zones.insert(0, zones.pop(screen_index))
There was a problem hiding this comment.
Anyways, I approved the PR, but this would be a visual nicety to have. It makes sense to have the "always persistent" uneditable zone at the top of the list always.
There was a problem hiding this comment.
Has this functionality changed during this PR? I'm inclined to open an issue and change this in a different changeset. Also, the backend should probably be changed to have consistent ordering.
There was a problem hiding this comment.
Well, yeah, it changed in the sense that now Screen is always at the bottom as opposed to always being at the top.
I consider it a minor UI regression, because I think most brainframe users have gotten used to the screen being right at the top
There was a problem hiding this comment.
This should be resolved now
| @@ -0,0 +1,136 @@ | |||
| from typing import List, Optional | |||
There was a problem hiding this comment.
Which file was it from? I can manually diff it on my end to try to provide feedback
| self.polygon_is_valid_signal.emit(True) | ||
|
|
||
| def on_frame(self, frame: ZoneStatusFrame) -> None: | ||
| # Only change the video frame if we're in the process of creating a new zone |
There was a problem hiding this comment.
Does this mean the video freezes when you're drawing a zone? How have I never noticed this??
There was a problem hiding this comment.
Note that this has not changed during the PR, just moved.
No, the super().on_frame clears all elements from the frame, but we want those to stay while we're editing. The else here ensures that the new frame is drawn.
I'll admit the comment is confusing though, so I'll change it
brainframe_qt/ui/dialogs/task_configuration/widgets/video_task_config.py
Outdated
Show resolved
Hide resolved
BryceBeagle
changed the title
This PR is the first step to allowing zones to be edited without deletion/recreation.
Right now, you can only add new points. (This means the changes are pretty useless for Lines). We will need future changes to delete points, and to move points.
Addresses #181