[WIP] - Multicam Display Support #1183

BryonLewis · 2022-02-24T14:16:03Z

Restructured the PRs for multicam into different feature branches. Renamed a branch and modified some stuff to make it a bit more clear what is happening.

Closes #1094.

BryonLewis · 2022-02-25T20:55:00Z

I need to update the resizing of the windows based on the control size modification. Currently it is using a number from a frame behind. You can see this by collapsing/expanding the timeline area. If you collapse it and resize the window slightly you'll see it resize properly. The resize Observer is not calculating the proper height it is using the height before the timeline panel collapses down.

* WIP * Doneish * WIP

BryonLewis · 2022-03-07T19:52:13Z

Through TrackStore development found another issue:
Reloading datasets after import broke the geoViewer references. Not exactly sure but I think it's because it calls a re-initialization of the cameras on top of existing values. It seems throwing a clearing function into the useMediaController where all of the base references are reset to their default values fixes this. I would like to know a bit more about what is going on and how it fixes it.

BryonLewis · 2022-03-11T20:05:52Z

List of changes since the merge:

Modified using symbols back to using strings. This has to do with typescript support for strings vs symbols and that we should have to update to a later version of TS for the symbol support for Record<symbol, data>
When doing a reload on a page by either importing new annotation data or when a piplined had finished the system would reload properly but the annotations wouldn't show up. I believe this had to do with creating a new 'symbol' with the exact same name as the one before. The clear() function which resets the geoViewer references and symbols seemed to fix this issue when the reload happened. The other option would be to prevent reloading of the layers when we reload all of the annotation data. This may be a bit difficult because reloading the annotation data means that the camera information is reloaded again.

subdavis

I don't know if some of this is supposed to work, so I'm just noting it here:

Continuous mode seems broken. When you start in continuous, then move into another camera, the "empty" annotation that gets created isn't removed, so if you're doing continuous, you end up with a bunch of detections with no geometry.
Pressing r to recenter is broken

I ignored the trackstore.

subdavis · 2022-03-21T18:02:58Z

client/dive-common/components/Viewer.vue

+export interface ImageDataItem {
+  url: string;
+  filename: string;
+}
+
+function loadImageFunc(imageDataItem: ImageDataItem, img: HTMLImageElement) {
+  // eslint-disable-next-line no-param-reassign
+  img.src = imageDataItem.url;
+}


This is identical to the default prop value. Why is it specified here?

I believe it's a remnant of the refactor, I'll remove it.

subdavis · 2022-03-21T18:06:58Z

client/dive-common/components/Viewer.vue

+    const defaultCamera = ref('default');
+    const selectedCamera = ref('default');


I think we need to do with with something other than strings. Using the reserved keyword "default" to name a camera causes the viewer to break.

Would the suggestion be to use a combined type for the camera references? null | string where null is used in the case of a single camera? Then strings are used for multicameras.

subdavis · 2022-03-21T18:08:48Z

client/dive-common/components/Viewer.vue

    const datasetType: Ref<DatasetType> = ref('image-sequence');
    const datasetName = ref('');
    const saveInProgress = ref(false);
-    const videoUrl = ref(undefined as undefined | string);
+    const videoUrl = ref({ default: null } as Record<string, null | string>);


It's subtle, but there are two empty conditions here when there should only be one.

Either the key is missing, or the key is set to null. So you have to compare against both null and undefined. It would be more strict to choose a single empty condition, which is that if no videourl exists for a camera, its key is not in the map at all, so you only have to compare against undefined.

client/dive-common/components/Viewer.vue

subdavis · 2022-03-21T18:34:01Z

client/dive-common/use/useModeManager.ts

+      let track = trackMap.get(selectedTrackId.value);
+      if (selectedCamera.value !== 'default') {
+        track = camTrackMap[selectedCamera.value].get(selectedTrackId.value);
+      }


I don't like having consumers responsible for all this logic, it's just more for bugs.
Possibly not part of this PR, but i think we need to

Get rid of trackMap and replace it with camTrackMap

Replace everywhere that access the trackMap with a getTrack(map, id, camera) call that properly throws an error if either the camera or id don't exist.

I've done this in the trackStore modification branch. There really is no longer a trackMap, it's all camMap used in conjunction with different getTrack functions. Except it's a little more complicated than a single getTrack function, because getTrack(map, id, camera) doesn't cover all needs, sometimes you just need the first trackId that's available and don't care what camera is selected, other times you need a conglomerate of all tracks across all cameras to display (in the case of the event Viewer for a selected track). Finally you need a list of all available tracks to iterate over (For setting confidence pairs on all tracks regardless of camera, or setting track wide attributes)

Got it, I remember talking about this now. If we need some new functions or a more complicated getTrack that makes sense. I just don't want each function that needs to get a track to have to implement its own error handling / null checking.

subdavis · 2022-03-21T18:36:48Z

client/dive-common/use/useModeManager.ts

+          if (!track) {
+            return;
+          }


What is this doing? We're inside an if (track) block already.

FWIW, that if (track) is also probably wrong because it implies that the else case is a valid execution path when it should probably be a runtime error.

it's a typescript oddity that it believes that track can be undefined even though the forEach is inside of an if (track). I'll see what I can do about it. If not in this PR it should be resolved in the next when there is better way to get the track from the camMap.

it's a typescript oddity

This would go beyond "oddity"--either there's some weird dynamics in this code that we're missing (perhaps due to lexical scoping of the forEach() callback?), or there's a typescript bug. What error do you get when you omit the if block? You can also use this technique (https://stackoverflow.com/a/66124742/1886928) to force the compiler to tell you what it thinks the type of track is at that point in the code.

It believes that within the forEach that track can be Track | undefined and doesn't want to use it as an argument to recipe.update

Update: Found an issue related exactly to this, marked as a duplicate to an issue that's still open:
microsoft/TypeScript#29916

And how about outside of the forEach callback (3 lines up, on the track.canInterpolate() invocation)?

It's fine there and is known as a track. I've updated my last comment and it might have been too late. But there is an issue exactly like this and has a simplistic demo showing the problem. I think I've seen this before as well.
microsoft/TypeScript#29916

Thanks for looking into this.

I know this is nitpicky, but it happens from time to time. When typescript misbehaves, I think it would be better to put in a // @ts-ignore than to insert code that handles what we know to be an invalid execution path. That return should be unreachable, and I wouldn't blame a reasonable developer for seeing that in the future and ripping it out because it looks like it shouldn't be there.

And because you don't want to disable type checks for that whole line, you can probably add something like this:

// @ts-ignore const _track: Track = track; const changes = recipe.update(eventType, frameNum, _track, [data], key);

Was implementing and did a quick search for the eslint-disable command and found this:

apparently my mind isn't deteriorating as quickly as I thought, we have run into this before.

You can also use a type assertion here ("casting" with the as operator) to avoid having to disable the ts-lint rules.

subdavis · 2022-03-21T18:38:24Z

client/dive-common/use/useModeManager.ts

  trackMap: Map<TrackId, Track>;
-  mediaController: Ref<MediaController>;
+  camTrackMap: Record<string, Map<TrackId, Track>>;


I don't think we should have trackMap anymore.

subdavis · 2022-03-21T18:44:11Z

client/dive-common/components/Viewer.vue

+    watch(controlsRef, (previous) => {
+      if (previous) observer.unobserve(previous.$el);
+      if (controlsRef.value) observer.observe(controlsRef.value.$el);
+    });


Can you add a comment about what causes this watch to fire? I think it's shadowing the value of the wrapping v-if="progress.loaded", and I'm fine with doing it like this, I just want to make sure that's what's happening.

subdavis · 2022-03-21T20:49:22Z

Maybe do the easy stuff and let's resolve the harder stuff later?

'default' reserved keyword is an issue, but not critical.
trackMap is taken care of in your next PR
Can think about doing a schema migration for representing single and multi-cam data in a consistent way after the other stuff works.

BryonLewis · 2022-03-22T17:39:48Z

I've switch to 'singleCam' for the identifier for a singleCamera. Right now it will conflict if there is a multiCam with the name of 'singleCam'. It will be easier to resolve after I re-base my trackstore PR. There all trackMaps are within a camMap meaning I can default to camera easier.
I think in the next PR I can also modify the Loading mechanism to remove the duplicate logic because the change to have camMap: Map<string,<Map<trackId, Track>>> be the holder of all tracks instead of this ridiculous thing I did temporarily here to get something to work.

The videoUrl change should cause the VideoAnnotator to throw an error if it tries to load the VideoAnnotator Component with a non-existent camera

subdavis

wooo

BryonLewis and others added 14 commits February 28, 2022 10:19

figuring stuff out by breaking it

6088cb1

need to redo useMediaController

edc4e4c

getting useMediaController to use multiple cameras

16e7cb6

some basic loading working

f316d48

fixing some css

403feb1

adding in multi-data-loading

ba1ae4c

Track modifications for multicam

243b339

some minor fixes

1eb11d5

minor build changes

f37b45f

build lib fix

d20508f

fixing build finally

30a9579

Variation on MultiCam Media Controller (#1171)

6ae400d

* WIP * Doneish * WIP

adding icons and keyboard shortcuts

bd9d973

fix emit

910b53e

BryonLewis force-pushed the multicam/display branch from 630989f to 910b53e Compare February 28, 2022 15:22

BryonLewis added 3 commits February 28, 2022 11:41

modifying symbols to strings for TS

d727658

build fix

f4adf57

fixing controls collapse sizing

d21d462

reloading syncing

0ae6e41

waxlamp linked an issue Mar 11, 2022 that may be closed by this pull request

Multi-Cam Simultaneous Display [FEATURE] #1094

Open

BryonLewis requested a review from subdavis March 14, 2022 19:22

subdavis suggested changes Mar 21, 2022

View reviewed changes

BryonLewis added 3 commits March 22, 2022 11:23

addressing comments

647c423

additional fixes

7a87b6c

switching to singleCam for default name

35f1d5c

changing to 'as Track'

31185ad

BryonLewis requested a review from subdavis March 23, 2022 16:56

subdavis approved these changes Mar 23, 2022

View reviewed changes

BryonLewis merged commit af5532f into multicam-support Mar 23, 2022

BryonLewis deleted the multicam/display branch March 23, 2022 19:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] - Multicam Display Support #1183

[WIP] - Multicam Display Support #1183

BryonLewis commented Feb 24, 2022 •

edited by waxlamp

Loading

BryonLewis commented Feb 25, 2022

BryonLewis commented Mar 7, 2022

BryonLewis commented Mar 11, 2022

subdavis left a comment

subdavis Mar 21, 2022

BryonLewis Mar 21, 2022

subdavis Mar 21, 2022

BryonLewis Mar 22, 2022

subdavis Mar 21, 2022

subdavis Mar 21, 2022

BryonLewis Mar 21, 2022

subdavis Mar 21, 2022

subdavis Mar 21, 2022

BryonLewis Mar 22, 2022 •

edited

Loading

waxlamp Mar 22, 2022

BryonLewis Mar 22, 2022 •

edited

Loading

waxlamp Mar 22, 2022

BryonLewis Mar 22, 2022

subdavis Mar 22, 2022

subdavis Mar 22, 2022

BryonLewis Mar 22, 2022

waxlamp Mar 22, 2022

subdavis Mar 21, 2022

subdavis Mar 21, 2022

subdavis commented Mar 21, 2022

BryonLewis commented Mar 22, 2022 •

edited

Loading

subdavis left a comment

		const defaultCamera = ref('default');
		const selectedCamera = ref('default');

[WIP] - Multicam Display Support #1183

[WIP] - Multicam Display Support #1183

Conversation

BryonLewis commented Feb 24, 2022 • edited by waxlamp Loading

BryonLewis commented Feb 25, 2022

BryonLewis commented Mar 7, 2022

BryonLewis commented Mar 11, 2022

subdavis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryonLewis Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryonLewis Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subdavis commented Mar 21, 2022

BryonLewis commented Mar 22, 2022 • edited Loading

subdavis left a comment

Choose a reason for hiding this comment

BryonLewis commented Feb 24, 2022 •

edited by waxlamp

Loading

BryonLewis Mar 22, 2022 •

edited

Loading

BryonLewis Mar 22, 2022 •

edited

Loading

BryonLewis commented Mar 22, 2022 •

edited

Loading