Video: whole frame level classification tool - #389
Conversation
|
Hi @wk9874 @praksharma please give this PR a go and let me know what you think |
abdullah-ukaea
changed the title
…ting the same fields.
… the frame label tool from allowing duplicate labels on one frame.
…nywhere on the image.
…f a custom-styled button.
… the already labelled warning toast.
14abefe to
de2cce6
Compare
|
Ready for review now @jblake42 |
|
From a functionality point of view this does not work in light mode I haven't looked at the code yet but my guess is it doesn't use spectrum correctly as this normally handles this. I think having the actual image being annotated able to be moved underneath the label is not ideal as it can mask the label - I would say a more obvious position and size for the label is required |
|
I think having each added label as a separate badge would be cleaner and easier to parse as a user and also easier to manage from a UI perspective than a constantly growing and shrinking width single badge |
jblake42
left a comment
jblake42
left a comment
There was a problem hiding this comment.
Please see the comments left in the main thread.
There are no specific code issue per-se here - I am starting to get concerns relating to extensibility as I can see this code getting a little messy again with various tool specific logic in the contexts. At some point I think we should have a freeze of functionality in the video section and see if there is anything we can do to standardise how tooling can be added (similar to what was done for plotly).
We have added a couple of non-annotorious tools now and there are still huge changes happening in the shared contexts etc.
Non of this is blocking for this functionality but I would like this to be a reasonable priority as it seems to be growing tech debt IMO
| tool: DrawingTool, | ||
| ): AnnotoriousDrawingTool { | ||
| return tool === "point" ? "rectangle" : tool; | ||
| return tool === "point" || tool === "frame" ? "rectangle" : tool; |
There was a problem hiding this comment.
I raised this last time as a risk and it has happened in the next PR - I worry about the architecture when we are having to tack on extra bits in logic to handle new tooling. I personally think from an extensibility point of view we have to consider how tooling is abstracted and how that can interface with the base tooling annotorious provides.
Otherwise I think we are going to get to the point where the video code gets very messy. I do understand this is hard when trying to interface with existing frameworks
| left={frameLabelPopupPoint.x} | ||
| top={frameLabelPopupPoint.y} | ||
| > | ||
| {frameLabels.map((frameLabel) => ( |
There was a problem hiding this comment.
Another benefit of having multiple badges (as per my comment on the PR) is that clicking on one badge with just bring up that popup which I think is more intuitve
There was a problem hiding this comment.
agreed, the new functionality should have removed this now.
| export type DrawingTool = "rectangle" | "polygon" | "point" | "frame"; | ||
| export type ActiveDrawingTool = DrawingTool | null; | ||
| export type AnnotoriousDrawingTool = Exclude<DrawingTool, "point">; | ||
| export type AnnotoriousDrawingTool = Exclude<DrawingTool, "point" | "frame">; |
There was a problem hiding this comment.
More places where little bits of extra logic has to be added whenever a tool is added - the concern is all of this has to be remembered when a dev adds functionality.
There was a problem hiding this comment.
Apologies for bringing this up again but I think it is important.
There are a huge amount of changes to the session context (preferably should be fairly tool agnostic) for the addition of another tool. This again rings some alarm bells for me about extensibility - I would expect us to be getting to the point where our contexts are pretty locked in now and the tooling logic can be handled more locally. I would say we are pretty close to that point with the plotly tooling almost all logic can now be contained within the actual tool specific file.
jblake42
left a comment
jblake42
left a comment
There was a problem hiding this comment.
I am happy with this following our discussion relating to planning for an attempt to refactor. The changes made to the UI styling make it much clearer
2 participants
Adds a new Frame Label tool to the video annotation UI for whole frame classification, so you can tag an entire frame with a class (e.g. UFO / no UFO) instead of drawing a bounding box.
Closes issue #225