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

Allow to convert agglomerate skeletons to normal ones #7537

Merged
merged 11 commits into from
Jan 15, 2024

Conversation

daniel-wer
Copy link
Member

@daniel-wer daniel-wer commented Jan 9, 2024

I also fixed the initialization of the mapping list for agglomerate skeletons iff json mappings are present. This was broken due to a regression before. The mappings property of a layer might already be set when initially obtaining the layer information, so it's not sufficient to check for that when checking whether mappings (and agglomerates) were already loaded.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Open the test-agglomerate-file dataset
  • Enable ID Mapping and check that agglomerate_view_70 is listed
  • Enable agglomerate_view_70 and load an agglomerate skeleton
  • Rightclick on the skeleton in the tree list and select "Convert to Normal Skeleton"
  • Check that the proofreading icon disappears, the skeleton can be modified freely and the change persists after a reload
  • Check that the conversion option is not offered for normal skeletons

TODOs:

  • ...

Issues:


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

@daniel-wer daniel-wer self-assigned this Jan 9, 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.

Great :) I left a comment regarding the bug fix. maybe it's worth to incorporate this.

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.

Testing went very smooth 👍 I noticed only two things:

When trying to modify an agglomerate skeleton with the skeleton tool, this message appears (rightfully):

Agglomerate skeletons can only be modified when using the proofreading tool to add or delete edges.

There, you could add "Consider switching to the proofreading tool or converting the skeleton to a normal tree via right-click in the Skeleton tab." or something like this.

I wondered how undo/redo worked for this conversion. It turns out to work well 👍 I know that undo/redo is not supported in the proofreading tool, but one can switch to the skeleton tool and then undo stuff that was done in the proofreading tool. I assume that this is not new to this PR and also not too easy to fix. However, I wanted to mention it here since it caught my attention. The impact is hopefully not too bad? The proper goal would probably be to allow undo/redo in the proofreading tool...

@daniel-wer
Copy link
Member Author

Thanks for testing! I incorporated your first suggestion 👍 Regarding the second one, you are correct, that's already been the case before this PR. I'd second your opinion to rather support undo/redo for the proofreading tool in the near future :)

@bulldozer-boy bulldozer-boy bot merged commit 5ebdee9 into master Jan 15, 2024
2 checks passed
@bulldozer-boy bulldozer-boy bot deleted the convert-to-normal-tree branch January 15, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants