-
Notifications
You must be signed in to change notification settings - Fork 24
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
Make layer order editable via drag and drop #7188
Conversation
…tom-layer-ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice stuff! DND works very nicely 👍
I left some comments. The biggest one is with regard to the shader recompilation which I would like to avoid as much as possible.
And another thought: maybe the reordering UI should be disabled when the layer mode is additive because it won't make a difference in that case? Otherwise, I imagine this to be a bit misleading..
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
@@ -633,6 +633,22 @@ class PlaneMaterialFactory { | |||
), | |||
); | |||
|
|||
let oldLayerOrder: Array<string> = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever written to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I missed that. This was designed to catch cases where the user drops the color layer at the same location to avoid shader recompilation. I now added a line that keep a copy of the previous layer order 👍 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philippotto I think I addressed everything and the layer rendering is now only enabled when the cover blend mode Is turned on.
@@ -633,6 +633,22 @@ class PlaneMaterialFactory { | |||
), | |||
); | |||
|
|||
let oldLayerOrder: Array<string> = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I missed that. This was designed to catch cases where the user drops the color layer at the same location to avoid shader recompilation. I now added a line that keep a copy of the previous layer order 👍 .
frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent 👍 just check my one suggestion before merging :)
This PR makes the color layer settings sortable via drag. It adds a grabbing menu icon left to the enable/disable switch. Users can use this handle to adjust the order of the color layers which will affect the rendering output in case the cover mode is active. Afaik there is no difference for the additive blending mode.
The handle is only shown if there is more than one color layer.
URL of deployed dev instance (used for testing):
Steps to test:
Test 1:
Test 2:
Issues:
TODOs:
Wanted Feedback:
layerOrder
. This is not as precise as it could be, because the variable only includes color layer names. Thus maybecolorLayerOrder
would be more accurate and more understandable. But on the other hand, we might decide to also make the segmentation layer sortable and then the name would no longer be correct. I talked to Norman and his opinion is that the segmentation layer should not be sortable in the current state, as there is no real use-case right now for it and the segmentation layer will always be rendered on top; no matter whether one of the upper layers defines the pixel as covered by selecting cover blend mode or not. Therefore the current version does not include the segmentation layers being draggable / sortable. @philippotto what do you is the best name to keep for the field? In long termlayerOrder
might be smarter but more confusing right now andcolorLayerOrder
would be good at the moment, but bad once segmentation layers are also sortable which might come in the future.colorLayerOrder
Important Note
In case anyone comes across this pr while developing the pr that makes segmentation layers sortable: There is already a working version at commit 4817a49
Bugs:
(Please delete unneeded items, merge only when none are left open)