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

Use segment index for non-connected ad-hoc meshing #7244

Merged
merged 18 commits into from
Sep 18, 2023

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Jul 31, 2023

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Create new annotation, draw non-connected segment
  • Render mesh

TODOs:

  • @philippotto Call new route, use it to call ad-hoc meshing route
  • Check if a segment index exists? Seems to be no function currently

Issues:


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

@philippotto
Copy link
Member

I just pushed the front-end integration. Seems to work great 🎉

  • Check if a segment index exists? Seems to be no function currently

Where should this happen? Front-end or back-end or both? I adapted the code so that the new route is used iff a volume tracing exists without an editable mapping and without a fallback layer. However, it would probably be cleaner and less error-prone (?) if the VolumeTracing had a hasSegmentIndex property. Related to that, I have some questions (if you don't know the answers, @fm3 can probably chime in):

  • Can it happen that a volume tracing doesn't have a segment index even when the above condition is met (legacy tracings?)
  • What would happen if the new route is called if a segment index does not exist? Probably an error status code?
  • If I have a volume tracing with fallback segmentation, does a segment index still exist? I know that fallback segmentations don't have a segment index currently, but I'm wondering if an index still exists for the buckets that were added to the tracing itself.

@philippotto
Copy link
Member

One idea: The existing generateIsosurfaceImpl function in the IsosurfaceService still suggests neighbors by looking at the bucket surfaces. The front-end won't use that information if the new route was used. What do you think about adding a dontFindNeighbors boolean parameter, @frcroth? The front-end could set it to true if the stats are used and the back-end would simply return an empty array instead of traversing the bucket data.

@fm3
Copy link
Member

fm3 commented Aug 14, 2023

@philippotto Thanks for dealing with the frontend part of this :)

The VolumeTracing proto object has an optional bool hasSegmentIndex (None should be interpreted as false). I think it would make a lot of sense to use that in the frontend instead of re-implementing the heuristic that decides if a segment index should exist. The frontend already retrieves the proto object, right?

Can it happen that a volume tracing doesn't have a segment index even when the above condition is met (legacy tracings?)

Yes, legacy tracings have no segment index, even if the condition is met. They can be duplicated to add one (I think that sacrifices the version history, though).

If I have a volume tracing with fallback segmentation, does a segment index still exist? I know that fallback segmentations don't have a segment index currently, but I'm wondering if an index still exists for the buckets that were added to the tracing itself.

Currently, no segment index is created/maintained at all for volume tracings that have a fallback segmentation. However, when voxelytics starts writing segment index files, we will add this feature in webknossos as well.

@philippotto
Copy link
Member

Great, thank you! I incorporated the hasSegmentIndex property and adapted the logic in the front-end accordingly. I still have the requirement in place that no editable mappings may exist which is probably fine, because editable mappings require a fallback segmentation anyway right now. However, I'm wondering what the plan is for a segment index when editable mappings exist? Is it planned to be implemented, too?

@fm3
Copy link
Member

fm3 commented Aug 18, 2023

segment index when editable mappings exist? Is it planned to be implemented, too?

I don’t think there is a plan yet. It would certainly be nice to have it, probably the segment index file would be for the oversegmentation, and webknossos would have to gather the correct ids from there, depending on the id mapping

@frcroth
Copy link
Member Author

frcroth commented Aug 21, 2023

One idea: The existing generateIsosurfaceImpl function in the IsosurfaceService still suggests neighbors by looking at the bucket surfaces. The front-end won't use that information if the new route was used. What do you think about adding a dontFindNeighbors boolean parameter, @frcroth? The front-end could set it to true if the stats are used and the back-end would simply return an empty array instead of traversing the bucket data.

@philippotto Doing it the other way around makes the name less akward in my opinion. So it's "findNeighbors" right now. Currently optional defaulting to true, can also be non-optional if the frontend always sends this.

Also, do you have sample data to test this (or have you already tested it?)

@philippotto
Copy link
Member

One idea: The existing generateIsosurfaceImpl function in the IsosurfaceService still suggests neighbors by looking at the bucket surfaces. The front-end won't use that information if the new route was used. What do you think about adding a dontFindNeighbors boolean parameter, @frcroth? The front-end could set it to true if the stats are used and the back-end would simply return an empty array instead of traversing the bucket data.

@philippotto Doing it the other way around makes the name less akward in my opinion. So it's "findNeighbors" right now. Currently optional defaulting to true, can also be non-optional if the frontend always sends this.

Great 👍 I integrated the new parameter, but it doesn't work yet. I see the new parameter in the payload (in the devtools), but the backend still sends the neighbors (I added an assertion to test against that which is why the frontend will crash currently when requesting ad hoc meshes). Could you have a look?

Also, do you have sample data to test this (or have you already tested it?)

You can simply create a new volume annotation, brush a bit, save and then load an ad hoc mesh for the new segment. Before the "findNeighbors" flag, this worked fine 👍

@frcroth frcroth marked this pull request as ready for review August 21, 2023 15:25
@frcroth frcroth changed the title WIP: Use segment index for non-connected ad-hoc meshing Use segment index for non-connected ad-hoc meshing Aug 21, 2023
@frcroth frcroth requested a review from fm3 August 21, 2023 15:26
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.

Backend LGTM!

I noticed that loading/refreshing the ad-hoc mesh does not ensure a saved state in the frontend. @philippotto do you know if this is intentional? if not, could you add it?

@philippotto
Copy link
Member

Backend LGTM!

I noticed that loading/refreshing the ad-hoc mesh does not ensure a saved state in the frontend. @philippotto do you know if this is intentional? if not, could you add it?

Sorry for the late reply. I added this now 👍

@frcroth
Copy link
Member Author

frcroth commented Sep 4, 2023

@philippotto Can you assign someone for front-end review?

@philippotto
Copy link
Member

@normanrz @MichaelBuessemeyer whoever's first to review, feel free :)

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Code looks good and seems to work
Screenshot 2023-09-06 at 10 08 07

@frcroth frcroth merged commit 42ad163 into master Sep 18, 2023
1 check passed
@frcroth frcroth deleted the segment-index-meshing branch September 18, 2023 12:56
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.

Use segment-index for ad-hoc meshing to enable rendering non-connected segments
4 participants