-
Notifications
You must be signed in to change notification settings - Fork 21
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
Port ifremer features #1348
base: main
Are you sure you want to change the base?
Port ifremer features #1348
Conversation
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.
Did a first pass looking at the code and running it.
I mostly avoided looking closely at the track_3d_viewer/ folder files mostly because it's specific logic for the implementation.
Most of my changes are mentioned to make sure that the 3D implementation doesn't interfere or impact the current versions of data. Mostly hiding things that aren't needed unless you have a stereo configuration file.
<v-btn | ||
color="secondary" | ||
@click="tracks3d = !tracks3d" | ||
> |
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'm guessing this should only be visible when there is a stereoconfiguration file? Regular stereo probably won't support this viewing type? Is there some type of requirement for this to be enabled at all?
Also a quick look at it has the button visible in standard (non-multicam mode)
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.
On toggle this should reset the camera area for the display. The video/images are resized and subsequent calls to reset the camera reset it back to it's original size instead of the 50% size.
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.
Also since this is being used a toggle button it should probably have two states to indicate on vs off.
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 agree, that is common sense. I'll update this. Thank you
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.
this should probably be enabled only if at least one detection has the x,y,z attributes
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.
- make it visible only in stereo config mode
- enable it only if we have at least one detection with stereo3d_* attributes
wdyt?
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 think it should only be visible if you have stereo config mode (being that it is stereo with your specific configuration file to enable 3D). I don't know if we need both for the button to be available because I don't understand the full context behind this feature. I.E: Does it make sense to have a track3d visible if you don't have stereo3d attributes or if you are going to create them at some point?
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.
Yep, let me explain better this feature.
Basically when you activate the 3D mode, it is going to query every tracks that are not filtered out, and create 3d object if it makes sense to do so, e.g, the track contains at least one detection with the stereo3d_*
attributes with defined value. Otherwise it is going to show a gray background and nothing interesting since there's no 3D coordinates information.
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.
|
||
<v-row> | ||
<v-col class="py-1"> | ||
<v-switch | ||
v-model=" | ||
clientSettings.trackSettings.newTrackSettings.modeSettings.Track.stereoMatching" | ||
class="my-0 ml-1 pt-0" | ||
dense | ||
label="Stereo Matching" | ||
hide-details | ||
/> | ||
</v-col> | ||
<v-col | ||
class="py-1 shrink" | ||
align="right" | ||
> | ||
<v-tooltip | ||
open-delay="200" | ||
bottom | ||
> | ||
<template #activator="{ on }"> | ||
<v-icon | ||
small | ||
v-on="on" | ||
> | ||
mdi-help | ||
</v-icon> | ||
</template> | ||
<span>Help</span> | ||
</v-tooltip> | ||
</v-col> | ||
</v-row> |
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.
This should probably only be viewable when in multicam or stereo mode and there is a stereo configuration file to enable matching.
[TrackViewerSettings.name]: { | ||
description: 'Track Viewer Settings', | ||
component: TrackViewerSettings, | ||
}, |
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.
This should probably be moved into viewer and removed/added if the system is in multicam mode. No reason in adding this sidebar if not in multicam/stereo mode or even more specific, stereo with a configuration file.
You can check on how the multicam options are added to see how these options can be removed/added only when certain conditions are met.
@@ -2,7 +2,7 @@ | |||
import { join } from 'path'; | |||
import moment from 'moment'; | |||
import { | |||
computed, defineComponent, ref, Ref, | |||
computed, defineComponent, ref, Ref, reactive, |
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.
remove reactive, it's not used.
// Set default attributes for a stereo calibration configuration | ||
if (argCopy.value.jsonMeta.stereoConfigurationFile) { | ||
argCopy.value.jsonMeta.attributes = {}; | ||
|
||
// Create x, y, z attributes | ||
['x', 'y', 'z'].forEach((attributeKey) => { | ||
// ugly but necessary to avoid typescript error | ||
(argCopy.value.jsonMeta.attributes as Record<string, Attribute>)[attributeKey] = { | ||
belongs: 'detection', | ||
datatype: 'number', | ||
key: `detection_${attributeKey}`, | ||
name: attributeKey, | ||
values: [], | ||
}; | ||
}); | ||
} | ||
|
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.
should these attribute keys be more specific about being stereo3D attribute keys. There are other users who do use attributes like this already.
I may be convinced that anyone utilizing stereoConfigurationFile
will just know that these attributes are reserved.
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.
agree to update the naming. where is the best place to update the documentation about what attributes are "reserved"?
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.
does stereo3d_x, stereo3d_y, stereo3d_z looks ok to you?
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.
Yeah that would be perfect. If you want to indicate that these attributes in combination with a stereoConfiguration file are reserved that could be added here: https://github.com/Kitware/dive/blob/main/docs/UI-Attributes.md
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.
updated in the two last commits
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.
Thibault opened a PR for VIAME as well here: VIAME/VIAME#162
started addressing comments in 584394a |
Hey @BryonLewis |
I believe I'll need to re-review it. I never had any actual 3D display before (I could get the renderer up and see the new panel but there wasn't a visbile display of 3D tracks before). If I can find some time for testing to make sure the feature works properly when given some test data it should be good. Just been a bit busy with some other projects. |
Thanks for the update Bryon. I'm going to send you test data that hopefully should work out of the box to test the feature by the end of next week. (I'm on vacation right now) |
- Automatically add corresponding track on other camera - Add geometry/feature on other track - Add a new setting to enable automatic track/geometry creation
- add @kitware/vtk.js dep - display tracks and detections (3d pos using detection attributes) - color the tracks according to their types - highlight the currently selected track - highlight the detections corresponding to the current frame - reload view after leaving 3d viewer
this shutdowns useless errors when checking VTK.js
- fix track color lookup - handle showing only selected track - draw only filtered track - show cube axes actor - fix: show track by default - fix: features are no longer exposed on track since Kitware#1287 so we now use a different mechanism to get the feature set of a track - use getTracksMerged since we don't care about cameras in this mode, we just want the detections and their attributes
- align camera, camera mode, reset camera - show 3d/2d views at the same time - better track viewer settings menu (in sidebar) - use one single lut per track type - make detection glyph size configurable - make cube axes actor bounds configurable - draw detection glyph label with track id - fix: feature coordinates yz inversion
- show stereo matching only in multicam mode - show 3d track viewer toggle only in stereo config mode - trigger annotator resize on 3d track viewer toggling - add track viewer settings to right sidebar only in stereo config mode
9c220b8
to
d9bd775
Compare
bb9b2b5
to
275a436
Compare
Trying to revive this PR @BryonLewis |
275a436
to
9b5c777
Compare
keep one single import dialog for stereo
9b5c777
to
857d29d
Compare
No description provided.