-
Notifications
You must be signed in to change notification settings - Fork 15
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
Video single frame annotation #1019
base: master
Are you sure you want to change the base?
Conversation
@aneust Was this closed on purpose? |
@mzur Closed on purpose, Leane found some strange behavior I first could not reproduce. Think now it was caused by the settings. |
Ok, thanks. You could also keep this as a Draft PR while you keep working on the issue. |
Regarding the issue with the sketch lines: |
@lehecht Thanks, that was helpful! |
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
resources/assets/js/videos/components/videoScreen/drawInteractions.vue
Outdated
Show resolved
Hide resolved
- update computed properties for better readability
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.
Looks good to me now. Please request a review from @mzur.
I saw the mention and self-requested the review 😉 @lehecht You can also request the review from me when you are done. |
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.
Two comments about the UI:
-
The setting doesn't affect whole-frame annotations but it should.
-
To be more user-friendly, the "finish annotation" and "track annotation" buttons (which are disabled) could get titles like "Disable the single-frame annotation option to create multi-frame annotations" ("Disable the single-frame annotation option to track annotations"). This way users are reminded of what to do if they want to use the buttons again. You could implement this with
v-if
showing a completely different set of buttons which are always disabled and have the new title.
Otherwise it works great. See some comments about the code below.
this.pendingAnnotation.frames.push(this.video.currentTime); | ||
this.pendingAnnotation.points.push(this.getPointsFromGeometry(e.feature.getGeometry())); | ||
if (this.singleAnnotation && this.isPointDoubleClick(e)) { | ||
this.pendingAnnotationSource.once('addfeature', function (e) { |
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.
Please add the explanation:
// The feature is added to the source only after this event
// is handled, so remove has to happen after the addfeature
// event.
if (this.singleAnnotation && this.isPointDoubleClick(e)) { | ||
this.pendingAnnotationSource.once('addfeature', function (e) { | ||
this.removeFeature(e.feature); | ||
}); |
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.
Shouldn't you return here and skip the rest of the function?
import { computeDistance } from '../../utils'; | ||
|
||
/** | ||
* Mixin for the videoScreen component that contains logic for the draw interactions. | ||
* | ||
* @type {Object} | ||
*/ | ||
|
||
const POINT_CLICK_COOLDOWN = 400; | ||
const POINT_CLICK_DISTANCE = 5; | ||
|
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.
Since computeDistance
and the constants are used both in the image annotation tool and the video annotation tool, please create a new file for these and reuse the same function/constants in both tools.
@@ -199,13 +213,19 @@ export default { | |||
let lastFrame = this.pendingAnnotation.frames[this.pendingAnnotation.frames.length - 1]; | |||
|
|||
if (lastFrame === undefined || lastFrame < this.video.currentTime) { | |||
this.pendingAnnotation.frames.push(this.video.currentTime); | |||
this.pendingAnnotation.points.push(this.getPointsFromGeometry(e.feature.getGeometry())); | |||
if (this.singleAnnotation && this.isPointDoubleClick(e)) { |
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.
Don't you have to check this.isDrawingPoint
here, too?
if (this.singleAnnotation) { | ||
if (this.isDrawingPoint) { | ||
if (this.isPointDoubleClick(e)) { | ||
this.resetPendingAnnotation(this.pendingAnnotation.shape); | ||
return; | ||
} | ||
this.lastDrawnPointTime = new Date().getTime(); | ||
this.lastDrawnPoint = e.feature.getGeometry(); | ||
} | ||
this.pendingAnnotationSource.once('addfeature', this.finishDrawAnnotation); | ||
} |
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 feel all this can be added to the if
above when refactored appropriately. This also would add the ´return` I asked for above.
@@ -52,6 +52,10 @@ | |||
<div class="sidebar-tab__section"> | |||
<power-toggle :active="muteVideo" title-off="Mute video" title-on="Unmute video" v-on:on="handleMuteVideo" v-on:off="handleUnmuteVideo">Mute Video</power-toggle> | |||
</div> | |||
|
|||
<div class="sidebar-tab__section"> | |||
<power-toggle :active="singleAnnotation" title-off="Enable Single-Frame Annotation" title-on="Disable Single-Frame Annotation" v-on:on="handleSingleAnnotation" v-on:off="handleDisableSingleAnnotation">Single-Frame Annotation</power-toggle> |
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 title is not very helpful. What about "Enable always creating single-frame annotations" (and "Disable ...")?
Add a settings option to the video annotation tool to automatically finish new annotations after the first frame
closes #786