Skip to content

Desktop Settings, VIAME CSV Parser, Pipeline Runner#424

Merged
subdavis merged 12 commits intomasterfrom
desktop/settings
Nov 16, 2020
Merged

Desktop Settings, VIAME CSV Parser, Pipeline Runner#424
subdavis merged 12 commits intomasterfrom
desktop/settings

Conversation

@subdavis
Copy link
Copy Markdown
Contributor

@subdavis subdavis commented Nov 4, 2020

Adds a settings page and starts to integrate with VIAME.

fixes #432

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Nov 4, 2020

See #417 for screensots

Base automatically changed from desktop/json-save to master November 5, 2020 18:32
@subdavis subdavis changed the title [WIP] Desktop/settings Desktop Settings, VIAME CSV Parser, Pipeline Runner Nov 10, 2020
@subdavis subdavis marked this pull request as ready for review November 10, 2020 17:26
@subdavis
Copy link
Copy Markdown
Contributor Author

This PR does not include a windows implementation or even platform switching logic. However, since this thing is already huge, I'm not inclined to include those at this time.

@subdavis subdavis requested a review from BryonLewis November 10, 2020 17:34
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.

You got the core functionality there pretty quickly. The web testing went really smooth and didn't notice any effects there (obviously you didn't really touch the web side).
These are just minor things that I've noticed might need to be updated/changed. Feel free to ask me any questions about my comments, I know that they aren't always clear.

@subdavis
Copy link
Copy Markdown
Contributor Author

subdavis commented Nov 11, 2020

@BryonLewis you were right about pretty much everything except gitignore: that would exclude the assets from getting published to NPM.

  • Addressed comments in c64252a
  • Tested polygon parsing, head tail, etc. with your suggestions and it looks like it works.
  • More robust test suite is needed.

FYI: yarn serialize viame annotations.csv is now available for this app.

Comment on lines +151 to +163
const coords: number[][] = [];
const polyList = poly[2].split(' ');
polyList.forEach((coord, j) => {
if (j % 2 === 0) {
// Filter out ODDs
if (polyList[j + 1]) {
coords.push([parseFloat(coord), parseFloat(polyList[j + 1])]);
}
}
});
geoFeatureCollection.features.push(_createGeoJsonFeature('Polygon', coords));
}
});
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.

Just a reminder to eventually add in some better checking here for poly being an array with an even number of coordinates. (I think that should be a requirement)

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.

Captured in #444

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.

All the fixes are tested and work well.
There is just a minor thing related to using Strings as Detection/Track Attributes. I believe right now certain values will return NaN.

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.

Pulled/Tested and now string based attributes show up properly.

@subdavis subdavis merged commit 5eb9372 into master Nov 16, 2020
@subdavis subdavis deleted the desktop/settings branch November 16, 2020 19:28
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.

[FEATURE] Run pipelines with desktop client

2 participants