imavid improvements + e2e #3893
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sashankaryal
changed the base branch from
develop
to
feat/video-frames-tagging
sashankaryal
force-pushed
the
refactor/imavid-e2e
branch
from
98a33d8
to
cffaacf
Compare
sashankaryal
force-pushed
the
feat/video-frames-tagging
branch
from
f2a399f
to
8b0d059
Compare
sashankaryal
changed the base branch from
feat/video-frames-tagging
to
release/v0.23.1
Closed
Comment on lines
+391
to
+401
| if (isImaVidLookerActive) { | ||
| // assuming we're working with only one sample | ||
| ( | ||
| lookerRef?.current as unknown as ImaVidLooker | ||
| )?.frameStoreController.store?.updateSample( | ||
| samples[0]._id, | ||
| samples[0] | ||
| ); | ||
|
|
||
| lookerRef?.current?.updateSample(samples[0]); | ||
| } |
There was a problem hiding this comment.
Can an instance check be used? ImaVidLooker can be added to the fos.Lookers union type
Suggested change
| if (isImaVidLookerActive) { | |
| // assuming we're working with only one sample | |
| ( | |
| lookerRef?.current as unknown as ImaVidLooker | |
| )?.frameStoreController.store?.updateSample( | |
| samples[0]._id, | |
| samples[0] | |
| ); | |
| lookerRef?.current?.updateSample(samples[0]); | |
| } | |
| const looker = lookerRef?.current; | |
| if (looker instanceof ImaVidLooker) { | |
| looker.frameStoreController.store?.updateSample( | |
| samples[0]._id, | |
| samples[0] | |
| ); | |
| looker.updateSample(samples[0]); | |
| } |
There was a problem hiding this comment.
Maybe a looker.updateSamples method could be added? Only one sample can be updated currently, but this would future proof selection/tagging changes and avoid accessing internal Looker data structures
Comment on lines
+348
to
+350
| if (necessaryFrameRange[1] < necessaryFrameRange[0]) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Should this be possible? It looks like getLookAheadFrameRange should always return a valid range.
sashankaryal
force-pushed
the
refactor/imavid-e2e
branch
from
cffaacf
to
05cdb31
Compare
Closed
benjaminpkane
approved these changes
Dec 11, 2023
benjaminpkane
pushed a commit
that referenced
this pull request
Dec 20, 2023
* more range check validation * fix tagging * remove log * fix imavid regression * upgrade dep versions * add watermarking option in image generator * fix frame one-off frame sync bug * move dynamic groups spec to groups folder * move dynamic groups spec to groups folder * add more data-cy attributes * differentiate grid and modal taggers * add speed controller * add toggleRenderFramesAsVideo * rename vars * add imavid tests * add screenshots * add data-cy to pathvalueentry * add tagging tests * remove unused vars * add modal tagger pom * add label count and sample count getters in siderbar * reset popups before play/pause
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR: