Skip to content

import fix and some animation for importing#874

Merged
BryonLewis merged 6 commits intomainfrom
desktop/bugfix-871
Aug 3, 2021
Merged

import fix and some animation for importing#874
BryonLewis merged 6 commits intomainfrom
desktop/bugfix-871

Conversation

@BryonLewis
Copy link
Collaborator

@BryonLewis BryonLewis commented Jul 30, 2021

Fixes #871
Fixes #873

871 - upated _importTrackFile so that it can accept a duser defined track file. If that track file is not included it will fall back to the default operation (This is required for stereo import of folders). If it is included and set to null/empty it will create an empty dataset.

873 - added a disabled state and spinner to the import for longer uploads. The previous delays were actually caused by the clearing of the tracks and not the import process itself. If modified the clearAllTracks function to remove the objects instead of iteratively calling removeTrack. This sped it up so on desktop it should be relatively quick and the longest part will be reloading the data.

@BryonLewis
Copy link
Collaborator Author

Updated to swap the clearAllTracks to clear the trackMap, trackIds then iterate through and remove all elements from the intervalTree. The other option for the interval tree is changing it to a let instead of const but I don't really feel great about doing that for something that is passed around.

This updated version handles the swapping between large imports much better. The majority of the time is spent loading instead of removing the older tracks.

@BryonLewis BryonLewis marked this pull request as ready for review August 3, 2021 17:52
Copy link
Contributor

@subdavis subdavis left a comment

Choose a reason for hiding this comment

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

  • I think the "Anntation File" input should be outlined and have the same padding to match the rest of the dialog. It doesn't align vertically.
  • It's not dismissable, so I can't, for example, import a dataset without annotations if one exists in the folder. I should be able to override to make it empty.

Screenshot from 2021-08-03 14-25-18

@BryonLewis
Copy link
Collaborator Author

Dismissable, prepend-icon-inner and small update to allow removal of annotation file if wanted.
image

Comment on lines +820 to +822
const foundTrackFileAbsPath = await _findJsonTrackFile(jsonMeta.originalBasePath);
const trackFileAbsPath = userTrackFileAbsPath !== undefined
? userTrackFileAbsPath : foundTrackFileAbsPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

userTrackFileAbsPath is annotated in the stub as string | null and then compared to undefined inside a conditional block where it's already been established as not null.

I'm going to venture a guess that you probably don't need to call _findJsonTrackFile at all in this function because it was called already in the pre-import step.

I'm going to venture a guess that because foundDetections requires trackFileAbsPath requires userTrackFileAbsPath which is always set inside the new conditional you added, that variable is doing no work at all and the block inside Look for other types of annotation files as a second priority is now unreachable code. Is that right?

Copy link
Contributor

@subdavis subdavis Aug 3, 2021

Choose a reason for hiding this comment

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

Yeah, I think we need to get rid of the test on line 823 for trackFileAbsPath so it's just "If it's a json file do this, otherwise if it's a CSV file do this".

The comment about look ing for other types of annotation files is misleading. there should no longer be any file discovery going on in this function. It's just handling the value of userTrackFileAbsPath

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the case of not importing stereoscopic data that is correct. In stereoscopic there is no precursor to finding the track files for each camera. That should probably be done as another PR where you define the folder and assign track files for each camera if needed.
If you import stereoscopic right now userTrackFileAbsPath is undefined and it should look for other annotation files associated with each camera. It does import those relative files but right now the display doesn't show them. Maybe this update should be apart of my stereoscopic viewing update.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks.

@BryonLewis BryonLewis merged commit 48b326c into main Aug 3, 2021
@BryonLewis BryonLewis deleted the desktop/bugfix-871 branch August 27, 2021 12:11
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.

[BUG] import button from viewer shows no loading progress, seems to freeze. [BUG] New dataset annotaiton import button ignores value

2 participants