Skip to content

Variation on MultiCam Media Controller#1171

Merged
subdavis merged 3 commits into
multicam/displayfrom
multicam-brandon
Feb 25, 2022
Merged

Variation on MultiCam Media Controller#1171
subdavis merged 3 commits into
multicam/displayfrom
multicam-brandon

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Feb 15, 2022

Summary of changes

  • Roughly 125 fewer lines
  • The motivation here was to make Multicam support more fully integrated. Instead of having different concepts for single and multi-cam code paths, this change Removes AnnotationWrapper and places its functionality inside useMediaController. That composition function treats all cameras the same: default is not a special case.
    • It removes the need to do {} as MediaController, and instead throws a proper exception if the mediaController is ever attempted to be used before initialization.
    • This change backs out our abuse of child -> parent binding using ref.value.mediaController
  • It also makes the "aggregate" media controller the primary way to interact with media. Like the earlier version, it's reactive and recomputes when cameras are added.
  • MediaController (the camera-specific one) extends AggregateMediaController. There were several functions previously only available on Aggregate that should have been camera-specific, like resetMapDimensions. LayerManager is camera-specific, and it gets passed a camera-specific mediaController, so it should only be calling camera-specific functions on MediaController.

Description of logic

  • Viewer initializes useMediaController (rather than a specific camera), which injects 2 static references for all children of Viewer. AgregateController and cameraInitializer.
  • Now, because aggregateController has been provide()ed one layer up, it's no longer necessary to pass through the template.
  • Cameras are created as components, and they register themselves with the injected cameraInitializer function during their setup(). They no longer have to worry about the internals of useMediaController.data, and are not given access to any state but their own.

Drawbacks to this approach

  • The arguments and return types to cameraInitializer had to be written excplicitly rather than inferred because of the provide/inject pattern.
  • ImageAnnotator and VideoAnnotator now rely on function hoisting to pass references to seek, play, pause, etc. to the initializer before those function definitions have executed. This isn't currently an issue because none of those functions are called except by human interaction, but it's worth noting.

Other caveats

This is not ready to merge, but it is ready to at least discuss.

  • I broke some CSS thing
  • I also broke camera selection

@subdavis subdavis requested a review from BryonLewis February 15, 2022 19:21
@subdavis subdavis marked this pull request as draft February 15, 2022 19:23
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

Just adding these here for reference.

Comment thread client/dive-common/components/Viewer.vue Outdated
Comment thread client/dive-common/components/Viewer.vue Outdated
Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

Small comments, which I may try to address after this initial merging. Making an internal list of things I need to update/fix like v-mousetrap and handling some weird switching of camera instances.

Comment thread client/dive-common/components/Viewer.vue
Comment on lines +301 to +307
// const offsetX = evt.clientX + 10;
// const offsetY = evt.clientY - 25;
// window.requestAnimationFrame(() => {
// if (imageCursorRef.value) {
// imageCursorRef.value.style.transform = `translate(${offsetX}px, ${offsetY}px)`;
// }
// });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this intended to be handled outside of the individual cameras?
There is some stuff that I probably need to do to get mouse-trap shortcuts working as well as proper behavior of being in a 'new' state and selecting a different camera before completing an annotation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I couldn't figure out what this did, so I disabled it to see if anything broke.

It doesn't appear to be doing anything. What is this for?

Copy link
Copy Markdown
Collaborator

@BryonLewis BryonLewis Feb 17, 2022

Choose a reason for hiding this comment

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

This handles the icon for new items attached to the mouse.
image

Only in creation mode does this icon come up.
I kind of like that it only appears on the camera you have selected. Makes it a bit clearer when creating a new track/annotation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks. Not sure why I didn't notice it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this now, and try and resolve the CI failure.

@subdavis subdavis marked this pull request as ready for review February 25, 2022 15:01
@subdavis subdavis merged commit 786f441 into multicam/display Feb 25, 2022
@subdavis subdavis deleted the multicam-brandon branch February 25, 2022 15:02
BryonLewis pushed a commit that referenced this pull request Feb 28, 2022
BryonLewis added a commit that referenced this pull request Mar 23, 2022
* figuring stuff out by breaking it

* need to redo useMediaController

* getting useMediaController to use multiple cameras

* some basic loading working

* fixing some css

* adding in multi-data-loading

* Track modifications for multicam

* some minor fixes

* minor build changes

* build lib fix

* fixing build finally

* Variation on MultiCam Media Controller (#1171)

* WIP

* Doneish

* WIP

* adding icons and keyboard shortcuts

* fix emit

* modifying symbols to strings for TS

* build fix

* fixing controls collapse sizing

* reloading syncing

* addressing comments

* additional fixes

* switching to singleCam for default name

* changing to 'as Track'

Co-authored-by: Brandon Davis <git@subdavis.com>
BryonLewis added a commit that referenced this pull request Apr 5, 2022
* figuring stuff out by breaking it

* need to redo useMediaController

* getting useMediaController to use multiple cameras

* some basic loading working

* fixing some css

* adding in multi-data-loading

* Track modifications for multicam

* some minor fixes

* minor build changes

* build lib fix

* fixing build finally

* Variation on MultiCam Media Controller (#1171)

* WIP

* Doneish

* WIP

* adding icons and keyboard shortcuts

* fix emit

* modifying symbols to strings for TS

* build fix

* fixing controls collapse sizing

* removal of camTrackMap

* removal of camTrackMap

* starting to add in tools for multiCam

* adding in tools beginning

* reloading syncing

* additional updates

* Adding editing mode clicking transfers, cleaning up getTrack and adding camera synchronization

* fixing keyframe issues, fixing tests, adding trackMerge for multicameras

* refactoring trackMap to camMap

* Adding the getAnyTrack function

* updating

* modifying for updated view

* adding in UI for multicam tools

* adding in user prompt for single camera pipelines on multicam data

* basic linking is working

* fixing tests

* updating camera settings

* fixing some removal logic

* mend

* began addressing comments

* Addressing more errors

* fixing more bugs

* Fixing lockCamera definition

* fixing camera swapping and track linking

* disabling trackMerge

* fixing pipelines/linking

Co-authored-by: Brandon Davis <git@subdavis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants