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

Cache image settings #208

Merged
merged 19 commits into from
Sep 28, 2018
Merged

Cache image settings #208

merged 19 commits into from
Sep 28, 2018

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Sep 3, 2018

See https://trello.com/c/22S56JnA/2-single-view-cache-settings

To test:

  • View image, edit settings:
    • Channels on/off, pick channel colours / LUTS, adjust sliders
    • Channels advanced settings - gamma, family etc.
    • Greyscale checkbox
    • Pan, rotate and zoom image
    • Z and T indexes
    • Z-projection
  • Move to a different image by clicking on thumbnail
  • Return to original image by clicking on thumbnail
  • Viewer should appear same as when you left it (same rendering settings, zoom, pan)
  • Try with multiple viewers. Not sure of the precise behaviour we want here but does it behave unexpectedly?

Known issues:

  • moving back and forwards several times between images without panning/zooming/rotating, pan/zoom/rotate gets reset.
  • projection doesn't get re-applied in some cases with multiple viewers.

@will-moore will-moore changed the title Cache image settings between viewers Cache image settings Sep 4, 2018
@jburel
Copy link
Member

jburel commented Sep 13, 2018

  • Click on FullRange the image is updated.
  • Move to another Image
  • Go back to the first Image
  • The Image displayed corresponds to Full Range. but the Min/Max button is selected

@jburel
Copy link
Member

jburel commented Sep 13, 2018

We could probably cache Histogram and Interpolate

@jburel
Copy link
Member

jburel commented Sep 13, 2018

When I switch back to an image with cache settings (unsaved settings), the Save button is displayed
It should be enabled if the settings do not match the saved settings.

@will-moore
Copy link
Member Author

@jburel The Min/Max, Full Range & Imported buttons probably shouldn't be 'radio' buttons where the last-clicked button is 'selected' since the settings don't continue to be 'Full Range' etc.
Currently the 'Min/Max' button is always selected, even if you've never set an image to Min/Max.

@jburel
Copy link
Member

jburel commented Sep 17, 2018

discussed with @will-moore
do not select any button e.g. min/max is always selected even if the window input != [min, max]

@will-moore
Copy link
Member Author

The Min/Max, Full Range & Imported buttons aren't 'selected' now (don't show state).
Also, the cached rendering settings, greyscale and Z/T are added to Undo queue so that the Save button is enabled. This also means you can use Undo to return to the unsaved state. NB: minor issue - to reuse existing methods for 'pasting' settings, the channel settings are added to the Undo queue separately from Z/T and greyscale. So, you may need 2 clicks on Undo to return to unsaved state.

To test:

  • Adjust rendering settings (including Invert, LUTS, family, coefficient etc)
  • Adjust Z/T and greyscale
  • Move to another image and back again.
  • See that all settings are applied and that Save and Undo are enabled.
  • Save should work as expected. Undo will undo the channels settings first, then Z/T & greyscale.

@jburel
Copy link
Member

jburel commented Sep 18, 2018

After clicking on any of the range button e.g. Min/Max it stays "selected". I think this should not be the case

The rest works nicely.

Copy link
Member

@jburel jburel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The various console.log will need to be removed too

if (cached.projection) {
// Don't need to update response since Z dimension-slider
// will update the ol3-viewer
// response.projection = cached.projection;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove commented out code

@jburel
Copy link
Member

jburel commented Sep 20, 2018

In multi-view it does not really work:

  • Double-click on the same image to enable multi-view
  • Do not synch the images
  • Turn off one channel for example
  • Select another image
  • The new image will be correctly displayed in the window with focus
  • Go back to the original settings.
  • It does not display the last viewed settings

@will-moore
Copy link
Member Author

Last commit should fix the multi-view issues described above

@rgozim
Copy link
Member

rgozim commented Sep 25, 2018

Latest commit solved the bug described by @jburel

@pwalczysko
Copy link
Member

In multi-view, when 2 viewports of the same image are open, then

  • if there are two different rnd settings in each of the two viewports, then showing another image in the viewport and going back to the orginal one will have just one rnd setting "remembered" for both viewports with the same image (I think this is acceptable - I guess you can "remember" just one rnd per image, right ?)

@pwalczysko
Copy link
Member

Bug:

- Projection seems to be not working too well, or the caching of it is not right

screen shot 2018-09-26 at 10 59 32

user-4, http://web-dev-merge.openmicroscopy.org/webclient/?show=image-79356

Workflow

- navigate to the image
- open it in iviewer
- change Rnd, selecting a LUT
- click on Projection button, drag the sliders to extreme max and min possible
- navigate to another image
- come back to the previous image (the one you did Projection on)
- observe disrcrepancy in what you saw before, and what you see now when you came back
- save the rnd -> observe discrepancy between thumb and full viewer (see screenshot)

@pwalczysko
Copy link
Member

pwalczysko commented Sep 26, 2018

I think the problem #208 (comment) stems from the fact that Projection is not cached, it is not even saved after Save button is clicked. This might seem "logical" to us, but for a user: I do not see why Projection is so "special".

Edit: Oh, no, Projection IS saved. Thus, #208 (comment) is a true bug.

@pwalczysko
Copy link
Member

pwalczysko commented Sep 26, 2018

moving back and forwards several times between images without panning/zooming/rotating, pan/zoom/rotate gets reset.

confirmed. this might be one to have a look again, but I don't think it is a blocker. It might get annoying when working with big images.

@pwalczysko
Copy link
Member

In summary, three issues, #208 (comment) (just one Rnd remembered per image, maybe a valid approach), #208 (comment) (projection - there is clearly a regression bug) and #208 (comment) - maybe for improvement later in another PR - obviously a deficiency, but one with which it would be possible to live

@will-moore
Copy link
Member Author

Fixed the toggling of Z-projection having no effect on the image-viewer.

@pwalczysko
Copy link
Member

Yes, the Z-projection problem is fixed.

I think that this is ready to merge FMPOV.

Maybe we could card the #208 (comment) ?

/**
* Applies the rendering settings, keeping a history of the old settings
*
* @param {Object} rdef the rendering defintion
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

* Applies the rendering settings, keeping a history of the old settings
*
* @param {Object} rdef the rendering defintion
* @param {boolean} for_pasting true if rdef supplied it for pasting of settings, false otherwise
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it/is

@jburel
Copy link
Member

jburel commented Sep 28, 2018

I have recorded the comment https://trello.com/c/c3BU4L1x/63-cache-settings-follow-up
@will-moore is working on an alternative solution. Recording in case @will-moore cannot finish the alternative option so it does not get forgotten

@jburel jburel merged commit 4dc3295 into ome:master Sep 28, 2018
@jburel jburel added this to the 0.6.0 milestone Nov 14, 2018
@will-moore will-moore mentioned this pull request Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants