Add option to hide tree edges #7296

MichaelBuessemeyer · 2023-08-24T18:21:27Z

This PR makes it possible for users to hide the edges of tree and also persist this setting.

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Open a tracing
Create two trees
Enable/Disable the edge visibility of the trees using the context menu in the skeleton tab
The nodes should always be visible (unless the whole tree is turned invisible)

TODOs:

Adapt Tracing Store to changes. I need your help here @fm3 or maybe @frcroth
~~[ ] Maybe also adapt wklibs client (as the trees have a new property)~~ decided to defer that
~~[ ] Adapt NML reading & writing~~ decided to defer that

Issues:

fixes #7089

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

Updated changelog
Adapted wk-libs python client if relevant API parts change

MichaelBuessemeyer · 2023-08-24T18:27:22Z

@fm3 (or @frcroth) I am struggling with the tracing store as the newly added skeleton update action updateTreeEdgesVisibility still needs to be added. The same goes for the new property each tree now has (if it does not yet exist the frontend sets a default for skeletons retrieved by the server).

The tracingstore currently throws the following error:

Could you please help me out here and add the necessary changes?

And maybe you know whether wklibs needs to be adjusted to comply with the newly added property.

fm3 · 2023-08-24T18:32:03Z

Sure, we can add that. Just to make sure I’m understanding this correctly: There is a new update action that sets for a specific tree the hideEdges boolean? So I would adapt the skeleton proto definition to reflect that, and add the update action in the backend?

I’m wondering if the boolean’s naming is ideal. Since trees also have isVisible, maybe edgesAreVisible = true? That would also match the name “updateTreeEdgesVisibility”. But I don’t have a strong opinion here.

Related: Should we also adapt the NML format? In this case, we could indeed also adapt the python libs to get/set this boolean. But I’d say adapting the libs not super important/urgent.

MichaelBuessemeyer · 2023-08-24T19:44:47Z

Sure, we can add that. Just to make sure I’m understanding this correctly: There is a new update action that sets for a specific tree the hideEdges boolean? So I would adapt the skeleton proto definition to reflect that, and add the update action in the backend?

I hope so, yes 😅. But we can talk about my changes a little before you start so you only make the necessary changes. You text me when you want to start :)

I’m wondering if the boolean’s naming is ideal. Since trees also have isVisible, maybe edgesAreVisible = true? That would also match the name “updateTreeEdgesVisibility”. But I don’t have a strong opinion here.

I noticed the same thing in the middle of coding these changes. Thus I simply applied your suggestion and renamed the property to edgesAreVisible.

Related: Should we also adapt the NML format? In this case, we could indeed also adapt the python libs to get/set this boolean. But I’d say adapting the libs not super important/urgent.

Oh yeah, that's right. I think I already adjusted the reading nml in the frontend. But the writer still needs to be adjusted 👍. I added the point to the ToDos.

fm3 · 2023-08-28T12:18:15Z

@MichaelBuessemeyer I added the backend changes, please see if they work for you as intended :)

Note that we decided against adapting NML (and thus the python libs) for the moment, compare https://scm.slack.com/archives/C5AKLAV0B/p1693224632337739?thread_ts=1684748763.542109&cid=C5AKLAV0B

Looks like the snapshot tests fail, possibly because the proto is now slightly different with the new property? Let me know if I need to change anything for that

…e-tree-edges

… into hide-tree-edges

…e-tree-edges

philippotto

looks good to me overall 👍 why didn't you need to adapt the edge_shader, though? don't the edges support alpha and 0.5 is then interpreted as 0, anyway? I think, you will need to change this, since the #7136 PR added alpha support for edges (because these can get half-transparent when going from one t value to another).

And another note: We have a generic update-tree-action which could have been used for updating the edgesAreVisible parameter. However, now that we have the dedicated update action, it's also fine :)

philippotto · 2023-09-05T09:02:11Z

frontend/javascripts/oxalis/model/helpers/nml_helpers.ts

@@ -721,6 +722,7 @@ export function parseNml(nmlString: string): Promise<{
              isVisible: _parseFloat(attr, "color.a") !== 0,
              groupId: groupId >= 0 ? groupId : DEFAULT_GROUP_ID,
              type: _parseTreeType(attr, "type", TreeTypeEnum.DEFAULT),
+              edgesAreVisible: _parseBool(attr, "edgesAreVisible", true),


this is only for parsing right? what about serialization? can be invoked via the trees-tab ("save selected trees as nml" or sth like that)

You are right, I did not take care of serialization. Internally we agreed to postpone the support of the new edgesAreVisible field in nmls. Thus I did not implement any NML support besides what is needed for typescript. This line is needed because the created object currentTree in line 712, because the field edgesAreVisible is required by this object. Alternatively, I could set the value to always be true, if you prefer this variation over already trying to parse a value that is not yet serialized, I would also be fine with that :)

Ah, I see, makes sense :)

frontend/javascripts/oxalis/geometries/materials/node_shader.ts

philippotto · 2023-09-05T09:10:44Z

The icon doesn't look good with the dark mode 🙈

…e-tree-edges

MichaelBuessemeyer · 2023-09-07T08:23:13Z

why didn't you need to adapt the edge_shader, though? don't the edges support alpha and 0.5 is then interpreted as 0, anyway?

Exactly 👍

I think, you will need to change this, since the #7136 PR added alpha support for edges (because these can get half-transparent when going from one t value to another).

Ok, I'll find a way 👍

And another note: We have a generic update-tree-action which could have been used for updating the edgesAreVisible parameter. However, now that we have the dedicated update action, it's also fine :)

🤔 I was following the way the visibility of the whole tree is updated. This has its own action. But this is potentially due to multiple of these update actions later being combined into a single action.
I can still undo these changes and use the regular tree update action, if you think, that's cleaner. This shouldn't take up much time, right?

fm3 · 2023-09-07T08:28:48Z

I think there is no harm in the dedicated update action.

I’m not certain anymore, but I think the update action for the tree visibility was introduced because of backwards compatibility. There are old update actions stored in fossildb, as they are applied lazily in the backend. When applying those, we would have to interpret the missing field, and consider whether it should mean the visibility is not changed, or is set to true. While introducing that logic would be fine by me as well, the dedicated action is fine for me as well. The only downside might be a busier history. Philipp, if you think it would be better to unify this into the update tree action, we can certainly do it as well.

MichaelBuessemeyer · 2023-09-07T13:08:12Z

I think, you will need to change this, since the #7136 PR added alpha support for edges (because these can get half-transparent when going from one t value to another).

This pr works perfectly together with the changes of the nd dataset pr. There were no changes needed from my side and it still works 💯

The icon doesn't look good with the dark mode 🙈

I also fixed the icon in dark mode and increased the size of the rectangles a little.

philippotto · 2023-09-07T13:46:44Z

I think there is no harm in the dedicated update action.

Yes, I agree.

I think, you will need to change this, since the #7136 PR added alpha support for edges (because these can get half-transparent when going from one t value to another).

This pr works perfectly together with the changes of the nd dataset pr. There were no changes needed from my side and it still works 💯

The icon doesn't look good with the dark mode 🙈

I also fixed the icon in dark mode and increased the size of the rectangles a little.

Awesome 👍 Will test now.

philippotto

Awesome, works great for me :)

add option to hide tree edges

f45b79b

MichaelBuessemeyer added backend frontend new feature labels Aug 24, 2023

MichaelBuessemeyer self-assigned this Aug 24, 2023

rename variable

0d98ccf

fm3 added 2 commits August 28, 2023 11:14

add edgesAreVisible in skeleton proto

22cf9aa

new update action in backend

f9ac975

MichaelBuessemeyer added 4 commits August 31, 2023 13:55

Merge branch 'master' of github.com:scalableminds/webknossos into hid…

11ad544

…e-tree-edges

Merge branch 'hide-tree-edges' of github.com:scalableminds/webknossos…

fdcbe1b

… into hide-tree-edges

fix tests and add missing arguments

0440fd8

Merge branch 'master' of github.com:scalableminds/webknossos into hid…

ce6a29b

…e-tree-edges

MichaelBuessemeyer marked this pull request as ready for review September 1, 2023 07:32

MichaelBuessemeyer requested a review from philippotto September 1, 2023 07:33

MichaelBuessemeyer and others added 5 commits September 1, 2023 10:30

fix backend tests

cef7141

set correct default for edge visibility in frontend

4bd70de

update doc string

22023c8

refresh snapshots

05f7a8d

Merge branch 'master' into hide-tree-edges

5f5760c

philippotto reviewed Sep 5, 2023

View reviewed changes

Merge branch 'master' of github.com:scalableminds/webknossos into hid…

2c8b2b3

…e-tree-edges

apply pr feedback

9f65135

Merge branch 'master' into hide-tree-edges

1282c26

MichaelBuessemeyer requested a review from philippotto September 7, 2023 13:39

philippotto approved these changes Sep 7, 2023

View reviewed changes

Merge branch 'master' into hide-tree-edges

39803d5

philippotto merged commit 1f07395 into master Sep 18, 2023

philippotto deleted the hide-tree-edges branch September 18, 2023 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to hide tree edges #7296

Add option to hide tree edges #7296

MichaelBuessemeyer commented Aug 24, 2023 •

edited

Loading

MichaelBuessemeyer commented Aug 24, 2023 •

edited

Loading

fm3 commented Aug 24, 2023 •

edited

Loading

MichaelBuessemeyer commented Aug 24, 2023

fm3 commented Aug 28, 2023 •

edited

Loading

philippotto left a comment

philippotto Sep 5, 2023

MichaelBuessemeyer Sep 7, 2023

philippotto Sep 7, 2023

philippotto commented Sep 5, 2023 •

edited

Loading

MichaelBuessemeyer commented Sep 7, 2023

fm3 commented Sep 7, 2023

MichaelBuessemeyer commented Sep 7, 2023

philippotto commented Sep 7, 2023

philippotto left a comment

Add option to hide tree edges #7296

Add option to hide tree edges #7296

Conversation

MichaelBuessemeyer commented Aug 24, 2023 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

Issues:

MichaelBuessemeyer commented Aug 24, 2023 • edited Loading

fm3 commented Aug 24, 2023 • edited Loading

MichaelBuessemeyer commented Aug 24, 2023

fm3 commented Aug 28, 2023 • edited Loading

philippotto left a comment

Choose a reason for hiding this comment

philippotto Sep 5, 2023

Choose a reason for hiding this comment

MichaelBuessemeyer Sep 7, 2023

Choose a reason for hiding this comment

philippotto Sep 7, 2023

Choose a reason for hiding this comment

philippotto commented Sep 5, 2023 • edited Loading

MichaelBuessemeyer commented Sep 7, 2023

fm3 commented Sep 7, 2023

MichaelBuessemeyer commented Sep 7, 2023

philippotto commented Sep 7, 2023

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented Aug 24, 2023 •

edited

Loading

MichaelBuessemeyer commented Aug 24, 2023 •

edited

Loading

fm3 commented Aug 24, 2023 •

edited

Loading

fm3 commented Aug 28, 2023 •

edited

Loading

philippotto commented Sep 5, 2023 •

edited

Loading