Skip to content
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

Add additional coordinates to segment index #7411

Merged
merged 37 commits into from
Feb 5, 2024

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Oct 25, 2023

Steps to test:

  • Open nd dataset and create some segments
  • Open segment context menu in viewport and see BB and volume info
  • In the segments tab, click on a group and see the segment statistics. Also test CSV export.
  • Make sure everything is working for 3D datasets, such as export
  • Make sure ad-hoc mesh rendering and its download is still working

TODOs:

Issues:


(Please delete unneeded items, merge only when none are left open)

  • Updated changelog
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases
  • Needs datastore update after deployment

@fm3
Copy link
Member

fm3 commented Nov 16, 2023

@frcroth do you know what happens if I created an ND volume annotation on master, brush some, and then upgrade to this change? Will everything explode or will this be handled somehow? It might be too much to ask to add another migration for this case (there aren’t many ND annotations in production yet), but if things get into inconsistent states, maybe we should just disable the segment index features for such annotations?

@frcroth
Copy link
Member Author

frcroth commented Nov 20, 2023

@frcroth do you know what happens if I created an ND volume annotation on master, brush some, and then upgrade to this change? Will everything explode or will this be handled somehow? It might be too much to ask to add another migration for this case (there aren’t many ND annotations in production yet), but if things get into inconsistent states, maybe we should just disable the segment index features for such annotations?

I validated that the annotations can still be viewed, but the segment index won't work anymore. Also I noticed that the front end does not provide additional coordinates for segment volume calculations leading to unhandled exceptions whenever one right clicks segments in the canvas.

@dieknolle3333
Copy link
Contributor

@dieknolle3333 dieknolle3333 marked this pull request as ready for review January 22, 2024 12:29
@philippotto philippotto changed the title WIP: Add additional coordinates to segment index Add additional coordinates to segment index Jan 22, 2024
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Getting there :)

@fm3
Copy link
Member

fm3 commented Feb 1, 2024

Good stuff! I added a comment about the json format.

Also, do you know what happens with nd volume annotations created before this PR? Do they have a broken segment index? Or don’t they have one? Do we need a migration for them, similar to the original #7376 ?

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

front-end looks good to me :) testing also went well.

@frcroth
Copy link
Member Author

frcroth commented Feb 5, 2024

Also, do you know what happens with nd volume annotations created before this PR? Do they have a broken segment index? Or don’t they have one? Do we need a migration for them, similar to the original #7376 ?

Well the segment index previously was invalid since all additional coordinates were mapped to the same bucket key. With this PR the previous segment index can't be used since the keys have changed. So I think if there are nd annotations with segment indices, it would be best to recalculate the segment index.

Maybe we need another route for that, since addSegmentIndex doesn't do anything if the segmentIndex already exists.

@fm3
Copy link
Member

fm3 commented Feb 5, 2024

As discussed in person, we found that no productively used volume annotations on nd datasets exist yet. So let’s add an sql migration archiving (state='Finished') any (experimental) existing annotations on nd datasets.

@frcroth frcroth requested a review from fm3 February 5, 2024 15:28
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

🚀

@frcroth frcroth merged commit b161b3d into master Feb 5, 2024
2 checks passed
@frcroth frcroth deleted the additional-coords-segment-index branch February 5, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segment Index + Stats for ND volume annotations
4 participants