Tri-State plane display in 3D viewport #5440

daniel-wer · 2021-04-29T16:06:42Z

Add a new display mode for the planes in the 3D viewport which hides them alltogether. Also, move the setting to the 3D viewport where it can be more easily discovered.
Additionally, I added a setting to hide the dataset bounding box in the 3D view as requested in the issue.

TODO:

Add an evolution that modifies the saved user settings and recommended configurations. (@fm3 Could you point me to an evolution that modifies json in the postgres database?)

URL of deployed dev instance (used for testing):

https://tristateplanedisplay.webknossos.xyz

Steps to test:

Open a tracing and use the different plane display settings. The settings can be found when clicking the cogwheel in the upper right of the 3D viewport. The new option (left) should hide everything related to the planes.
Test the setting to hide the dataset bounding box. The setting should be persisted.
Create a task type and activate the recommended settings. There should be an explanation which values can be used for the tdViewDisplayPlanes setting and changing it to something else should not validate.

Issues:

Updated (unreleased) changelog
Updated (unreleased) migration guide if applicable
Ready for review

fm3 · 2021-04-29T16:37:25Z

@daniel-wer sure :) You can find different json manipulations in conf/evolutions/{051-add-source-view-configuration.sql, 052-replace-segmentation-opacity.sql, 057-add-layer-specific-view-configs.sql, 063-novelUserExperienceinfos.sql}. I think most of them were written by @youri-k

philippotto

Very cool!! The UI looks top-notch 🕺

I only left some small questions. Will wait for the migrations now :)

philippotto · 2021-04-30T07:49:29Z

app/models/configuration/UserConfiguration.scala

@@ -26,7 +26,6 @@ object UserConfiguration {
      "dynamicSpaceDirection" -> JsBoolean(true),
      "displayCrosshair" -> JsBoolean(true),
      "scale" -> JsNumber(1),
-      "tdViewDisplayPlanes" -> JsBoolean(true),


Shouldn't there be two new lines for tdViewDisplayPlanes and tdViewDisplayDatasetBorders?

Or doesn't the dictionary need to be complete here? If so, we could think about removing all the default values here, since there are defined in the front-end, too?

To my understanding, these default values are unnecessary as they are defined in the frontend as well. @fm3 Do you see any reason why we would still need these defined in scala?
If not, I could go through the list, double check that the frontend default is the same as the backend one and remove them from the backend.

@youri-k can you answer that? I’m not sure about those defaults and their duplication. If you also don’t know for sure, we can dive into the code and figure out what’s redundant.

One thing to note is that the list is very incomplete already and contains unused settings like isosurfaceBBsize, layoutScaleValue, and renderComments.
I had an in-depth look in the frontend and there these backend "default" values are not needed. The frontend defines a default value for each setting and uses those unless they are overwritten by the user config that is fetched from the server (which happens after users changed settings).

Alright, after repeated deja-vus I found out that we had this exact discussion before 🙈
Issue: #3906
Discussion: #3895 (comment)

I'd propose to remove the backend default UserConfiguration in this PR once and for all. I don't see a reason why the defaults need to be duplicated in the backend.

I'd propose to remove the backend default UserConfiguration in this PR once and for all. I don't see a reason why the defaults need to be duplicated in the backend.

I agree with you. I don't see any benefits, as well.

I removed the entire UserConfiguration wrapper in the backend (it now only handles this as a JsObject).

I believe this fixes #3906

The evolutions look good :) Only open question: does the task-type part handle NULL values for the recommendedConfiguration correctly? Or does it go to its else branch then?

I checked that the WHERE recommendedconfiguration ? 'tdViewDisplayPlanes' evaluates to false if recommendedconfiguration is NULL or if the 'tdViewDisplayPlanes' key doesn't exist. I think these are all the cases we need to worry about. If the key exists, its value will be a boolean which is where the CASE comes into play.

frontend/javascripts/oxalis/view/td_view_controls.js

frontend/javascripts/oxalis/controller/scene_controller.js

normanrz · 2021-05-02T15:25:43Z

@daniel-wer While you're at it, could also do #5380?

…-state-plane-display

…configuration subkey of userconfiguration in evolution

daniel-wer · 2021-05-03T14:24:43Z

I wrote and tested the evolution to change tdViewDisplayPlanes from a boolean to the tri-state enum.
Also, I removed all entries of the backend user configuration. However, there is still an empty configuration: {} inserted into the default user configuration and now that the defaults are no longer needed the scala code can probably be cleaned up a little bit. @youri-k or @fm3 Could one of you have a look at that and review my evolution as well?

philippotto

Front-end looks perfect :)

philippotto · 2021-05-03T14:39:53Z

frontend/javascripts/oxalis/geometries/cube.js

@@ -48,8 +44,6 @@ class Cube {
    const color = properties.color || 0x000000;
    this.showCrossSections = properties.showCrossSections || false;

-    _.extend(this, BackboneEvents);


fm3

Backend/SQL LGTM

make tdview plane display setting tri-state and move it to tdview

542e753

daniel-wer requested a review from philippotto April 29, 2021 16:06

daniel-wer self-assigned this Apr 29, 2021

daniel-wer added 2 commits April 29, 2021 18:09

fix icon margins

8aa6f1e

update changelog

06277ee

daniel-wer added 2 commits April 29, 2021 19:31

also add a setting to hide dataset borders in the 3D view

405d34b

update changelog

f308917

philippotto reviewed Apr 30, 2021

View reviewed changes

daniel-wer added 5 commits May 3, 2021 12:16

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

ea787c5

…-state-plane-display

apply PR feedback and remove isosurface button from 3D view

79f189b

add evolutions to adapt tdViewDisplayPlanes from boolean to enum

418a0d0

bump schema version

3550234

remove default values from userconfiguration.scala and remove unused …

b20fe2b

…configuration subkey of userconfiguration in evolution

philippotto reviewed May 3, 2021

View reviewed changes

Remove backend-side structuring of user configuration

9d3515b

fm3 approved these changes May 4, 2021

View reviewed changes

daniel-wer and others added 2 commits May 5, 2021 10:01

Merge branch 'master' into tri-state-plane-display

63d4605

Merge branch 'master' into tri-state-plane-display

3801cda

fm3 merged commit 3005b90 into master May 5, 2021

fm3 deleted the tri-state-plane-display branch May 5, 2021 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tri-State plane display in 3D viewport #5440

Tri-State plane display in 3D viewport #5440

daniel-wer commented Apr 29, 2021 •

edited

Loading

fm3 commented Apr 29, 2021

philippotto left a comment

philippotto Apr 30, 2021

philippotto Apr 30, 2021

daniel-wer May 3, 2021

fm3 May 3, 2021

daniel-wer May 3, 2021

daniel-wer May 3, 2021

daniel-wer May 3, 2021

youri-k May 3, 2021

fm3 May 4, 2021 •

edited

Loading

daniel-wer May 4, 2021

normanrz commented May 2, 2021

daniel-wer commented May 3, 2021

philippotto left a comment

philippotto May 3, 2021

fm3 left a comment

Tri-State plane display in 3D viewport #5440

Tri-State plane display in 3D viewport #5440

Conversation

daniel-wer commented Apr 29, 2021 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

fm3 commented Apr 29, 2021

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fm3 May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanrz commented May 2, 2021

daniel-wer commented May 3, 2021

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fm3 left a comment

Choose a reason for hiding this comment

daniel-wer commented Apr 29, 2021 •

edited

Loading

fm3 May 4, 2021 •

edited

Loading