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

Set default view configuration in datasource properties json #4357

Merged
merged 51 commits into from
Mar 17, 2020

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Dec 6, 2019

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

for the evolutions:

  • after the evolution the segmentationOpacity of the configuration element should be gone
  • instead the old segmentationOpacity value should be the alpha value of a newly created entry in the layers dict with the name of the related segmentation layer as key
  • the reverse evolution should take the alpha value from segmentation layer entry in the layers dict and save it as segmentationOpacity
  • to confirm that the configuration entries are still correct, open the dataset edit view and look at the view configurations

for the disk defaults:

  • if a view configuration is supplied in the datasource properties json, it should take precedence over the normal default, but if a existing dataset config takes precedence over the disk default
  • e.g. change the alpha value of a color layer (default is 100)

Issues:


@youri-k youri-k added the backend label Dec 6, 2019
@youri-k youri-k requested a review from fm3 December 6, 2019 16:10
@youri-k youri-k self-assigned this Dec 6, 2019
@youri-k youri-k changed the title [WIP] Set default view configuration in datasource properties json Set default view configuration in datasource properties json Dec 13, 2019
@youri-k
Copy link
Contributor Author

youri-k commented Dec 19, 2019

@MichaelBuessemeyer this pr should also take care of #4378. Could you do the necessary frontend part? This involves the the dataset edit view and the rendering in the tracing view. Thank you 🙂

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.

Wow, cool stuff :)
Small thing: could you provide a small example json in the changelog / steps to test?
Also, reading the changelog, I think defaultViewConfigurationOpt is a user-facing field name? I’d prefer to go without the “Opt” in the json (that was just a shorthand for us related to the scala Option code)

@MichaelBuessemeyer
Copy link
Contributor

@youri-k If we want to

Instead use the alpha value of the SegmentationLayer

we need to add the configs of the segmentation layer to the other layer configs in the DatasetConfiguration under the property "layers". Currently, that's not the case and will be needed.

Can you please do the backend part of this and write a conversion script, that removes the "segmentationOpacity" from DatasetConfiguration and adds a layer configuration for the segmentation to the layers property of DatasetConfiguration.

@daniel-wer daniel-wer removed their request for review March 3, 2020 15:17
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.

LGTM! But as @fm3, I didn't test this again. Please do so before deploying (especially the case were view configurations already exist in the old format).

@MichaelBuessemeyer
Copy link
Contributor

@philippotto First of all, I am sorry that I missed merging this PR earlier.

Because I missed this, your newest PR reworking the rendering to only display the enabled layers when the GPU does not support rendering all layers created some "hidden" merge conflicts.

I tried resolving them and it looks fine now and everything works as expected on my local machine. Nevertheless could you please check the newest changes again and whether everything works on your machine? But I can ensure that the bug @fm3 found is resolved and does not appear in the current version of this PR.

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.

Thanks for resolving the hidden conflicts!

I just tested the branch, but something doesn't seem right to me:

Shouldn't this use the values I edited?

frontend/javascripts/oxalis/api/api_latest.js Outdated Show resolved Hide resolved
@MichaelBuessemeyer
Copy link
Contributor

@philippotto Thanks for finding this bug. I debugged it a bit further and it looks like the backend sends false information:

  • When I open the link of yours in a private tab and look at the network traffic the https://viewdefaultsfromdisk.webknossos.xyz/api/dataSetConfigurations/sample_organization/2017-05-31_mSEM_scMS109_bk_100um_v01-aniso call returns:
{
    "quality": 0,
    "layers": {
        "color": {
            "brightness": 0,
            "contrast": 1,
            "color": [255, 255, 255],
            "alpha": 100
        }
    },
    "highlightHoveredCellId": true,
    "fourBit": false,
    "interpolation": true,
    "renderMissingDataBlack": true
}

In my opinion the answer should have an alpha value of 80 and the color you set in the settings, shouldn't it?

@youri-k Could you please have a look at this issue/bug? I think it has something to do with the backend.

@youri-k
Copy link
Contributor Author

youri-k commented Mar 16, 2020

@MichaelBuessemeyer I found the bug and fixed it 🙂
Additionally, I found something interesting: When you open a new dataset, you get the current dataset default configuration, and instantly respond to the server with this default config as your new user config. When the default config changes afterwards, you won't notice the change because your user config takes precedence. I don't know if this intended, but it's the same on the master. My feeling would be to only save a new user config, if the user actually changes his dataset settings and not just copying the default config at the point of first opening the dataset.
/cc @philippotto @fm3

@MichaelBuessemeyer
Copy link
Contributor

@youri-k Awesome :D
Thanks for fixing the bug.
According to your thoughts, I think this is not an issue to discuss in this PR as this is how the behaviour has being for a long time. Could you please open a new issue for this?

@philippotto
Copy link
Member

Cool, then we can merge this tomorrow, right? @youri-k Can you take care of this (plus migration)?

@fm3 fm3 added the automerge label Mar 17, 2020
@bulldozer-boy bulldozer-boy bot merged commit fa422cd into master Mar 17, 2020
@bulldozer-boy bulldozer-boy bot deleted the view-defaults-from-disk branch March 17, 2020 10:19
@normanrz
Copy link
Member

Nice! Now we need to put this into voxelytics (and maybe wkcuber)

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.

Remove segmentationOpacity from DataSet Set initial layer color trough datasource-properties.json
6 participants