Lazy load shapes #351

will-moore · 2020-09-07T13:29:16Z

This addresses just the first 2 bullet-points from #335 (comment)

In this PR, when ROIs are paginated (over 500 ROIs), we load just those shapes that are on the current plane.
The ROI JSON includes shape_count so we can continue to display that.
When an ROI is expanded (either by clicking on the ROI, or when a shape within the ROI is selected), then the Shapes are loaded.

This PR needs ome/omero-marshal#69

To test:

Need to test all ROI/shape workflows: load, edit, delete, create, save etc.
Test on images paginated ROIs (over 500) and non-paginated.

plugin/omero_iviewer/views.py

Don't need to use BindingEngine in the view component

jburel · 2020-09-08T13:15:45Z

From last week discussion, I understood that if a shape in a roi is selected, then a movie either across z or t is played the shape(s) in that roi will be displayed if there is any on the plane. This is not the case.

will-moore · 2020-09-08T13:50:57Z

See description "This addresses just the first 2 bullet-points from #335 (comment)"
They are:

Load only the shapes on the plane that we can look at. The ROI count is loaded
When the user select a shape, the other shapes will be loaded

This doesn't include the next 2 points:

If user plays a movie, only the selected "loaded" shapes will be displayed
Keep the shapes not linked to a given plane in the display

Actually, it also does support the last point (no change needed)

Go back to the first step when the user stops playing the movie

jburel · 2020-09-08T14:38:36Z

@will-moore apologies. I got ahead of the game.

will-moore · 2020-09-09T13:44:23Z

That commit should cover the next bullet points (but not tested yet with "shapes not linked to a given plane":

If user plays a movie, only the selected "loaded" shapes will be displayed
Keep the shapes not linked to a given plane in the display

The user experience is definitely better than it was, but there remains an issue with the frame rate of image pixel data loading is different from the rate that ROIs are updating.
This is not related to this PR: it's an issue on existing iviewer (when ROIs are not paginated/lazy-loaded).

jburel · 2020-09-10T11:05:23Z

Previously selecting a ROI, will select all the shapes in that ROI. This means that when I start playing a movie. I have the "selected" bold border around the shape. Now when I select a ROI, the last shape only is selected, so when I play a movie the selection is gone

Edit: it depends where I click on the roi entry. If I click between the checkbox and the count, all shapes are selected.

jburel · 2020-09-10T11:12:58Z

I can imagine a user being annoyed by the chosen approach so we might have to rethink this use cases.
User has several tracks, he/she selects only one ROI (spent time unchecking the boxes) start playing the movie, the shapes for the select rois are displayed. He/she stops the movie to view a specific plane for example half-way through the tracks. At that stage, we are back to the first step, the user will have to unselect again the boxes before continuing playing the movie.

will-moore · 2020-09-10T14:22:22Z

OK, there's a couple of issues here:

Selection of all shapes in the ROI:
- The behaviour is always to try and select all Shapes when we click on the ROI in the table (not on the expand arrow).
- This is done sequentially, setting selected = true on each Shape in turn.
- Since the last shape is the last one to be selected, we end up browsing to that Z/T plane (This is probably NOT what we want?!)
- If we have all the Shapes already loaded (no pagination) then they all stay selected. BUT, if we're reloading Shapes for the new (last) Plane then only the last Shape is selected. Only a single shape_id is used to remember the previous selection (see previous PR):
  https://github.com/ome/omero-iviewer/pull/342/files#diff-b445f40c6859f02edacb925f6ab66d49R876
  and it is only the selection of this shape (that is loaded because it's on the new Z/T plane) that causes the ROI to expand and the other Shapes in that ROI to be loaded. So, we'd need to remember the multi-shape selection up until that point and select those shapes when they are re-loaded.
Hiding of Shapes
- The hide/show state of previously-loaded ROIs (and shapes) is lost when we load ROIs for a new plane.
- However, that is only part of the problem. You will also be seeing ROIs on the new plane that weren't even loaded when you started the movie (so you couldn't have hidden them - even if the hiding state was preserved). So, we're always going to risk seeing additional shapes appear on a new plane, even when we can preserve the hide/show state.

It seems with both these issues, we are in trouble because we wipe out existing ROI layer for the new plane, and re-create from scratch, instead of updating the current one. So maybe I'll try keeping the ROI layer and all ROIs that were previously loaded and simply add the new ones in when they're loaded.

Also I'll try to fix the ROI selection to NOT navigate to the plane of the last Shape. If the current plane has one of the Selected shapes on, don't change Z/T - otherwise navigate to the plane of the FIRST shape?

pwalczysko · 2020-09-15T09:51:18Z

Noted that the "Show" checkbox is not present on merge-ci atm.

will-moore · 2020-09-15T16:48:48Z

That last commit should fix the 'Show' checkbox.
I think this is ready for another round of testing.

Now, this should work for image with paginated ROIs (> 500)

Open ROIs tab - ROIs with shapes on the current plane are loaded - other shapes in those ROIs are not loaded
Click on a shape on the viewer to select it (expands the ROI in the table) - or manually expand the ROI in the table
This should load the other Shapes for that ROI
Can click on the ROI in the table to select all the Shapes
Can then play the movie. The selected shapes will be shown on each plane in turn (don't need to load new shapes while movie is playing)
When the movie stops, we load all the Shapes for that plane
Should be able to select another shape and repeat...

I also added a few notes to the docs/README and a diagram to try and retain some of the knowledge about how this all works, since it's not possible to keep it all in my head and takes a while to work out from the code.

jburel · 2020-09-17T13:27:37Z

I used https://merge-ci.openmicroscopy.org/web/webclient/img_detail/105665/
Show out of synch:

Go to the ROI tab, the regions are loaded
Untick the Show checkbox, select a couple of shape
Move to another page, all the shapes are selected but in that case, the Show is not selected

jburel · 2020-09-17T13:36:33Z

I used https://merge-ci.openmicroscopy.org/web/webclient/img_detail/139315/
ROI selection out of synch

untick Show
Click on the arrow to expand the rois, all the shapes are selected but not the rois

docs/README.md

jburel · 2020-09-17T13:39:43Z

I used https://merge-ci.openmicroscopy.org/web/webclient/img_detail/139315/

Select one roi, and play movie across z. This is now possible
Stop the movie at z=291. Regions are loaded. But some regions with no shapes on that plane are also loaded. The roi is not selected

docs/README.md

pwalczysko · 2020-09-17T16:41:53Z

Regions are loaded. But some regions with no shapes on that plane are also loaded. The roi is not selected

Can confirm @jburel 's observation. If you do not select any ROI and then play the movie, then only the ROIs which are on a given frame will be loaded once you stop the movie.
But, if you select a ROI first, then play a movie, then many ROIs load when you stop the movie at any given frame which have no shapes on that frame. If you expand such ROIs (which are loaded in the table, but not checked as "show", their shapes, as indicated, are not belonging to that plane, but some of the shapes are checked as "show" in the table and some are not, without any apparent logic.

Edit: Maybe the logic is, that any shape, which is on a plane on which some selected shape is (you selected a ROI with shapes before you started the movie), is loaded, irrespective of whether or not the viewer is on that plane ? There is some logic to this, but it is not helpful or expected.

will-moore · 2020-09-17T17:02:19Z

There's a couple of issues here.
I'm currently not discarding previously-load ROIs and shapes when you navigate to a new Plane.
So, the first plane you load, only ROIs with shapes on that plane will be loaded. But when you go to another plane, now you will have shapes from both planes. I could revert that. I didn't think that it would be confusing since I didn't expect the user to care. But I guess I'm also not getting any benefit from keeping them in hand.

There isn't yet a place in the JS model that stores the 'checked' state of an ROI checkbox. So when loading shapes for the ROI, there's no way to know whether they should be shown. So I'll have to store the ROI checkbox state.

will-moore · 2020-09-23T16:39:28Z

Having spent a fair bit more time on this, it's proving very hard to manage the selection state of Shapes while changing Z/T and loading new ROIs/Shapes.
The previous behaviour was to clear all shapes from the regions_info model class and the OpenLayers Regions layer when loading new ROIs for new Z/T.
But this loses all the selection and visibility info.
The alternative above was to NOT clear existing Shapes when new ones are loaded. But then we have the issue of having some ROIs/Shapes loaded that are not on the current Z/T plane.
ef3f2f2 is an attempt to use the regions_info.selected_shapes list to set the selected state of Shapes when they are re-loaded. However, this doesn't do the same for the OpenLayers Regions layer, so we get out of sync.
Also, when loading the ROIs/Shapes for a newly chosen Z/T plane, there is no trigger to expand the ROIs that have selected Shapes, so the shapes aren't loaded.

Probably there are fewer issues to deal with when we don't clear all the existing Shapes.
How much of a problem is it to have ROIs/Shapes loaded that aren't on the current Z/T plane?

pwalczysko · 2020-09-24T09:48:29Z

How much of a problem is it to have ROIs/Shapes loaded that aren't on the current Z/T plane?

It is not great in the sense that it is an inexplicable behaviour from users standpoint. For the smoothness of the workflow, I think it only becomes a problem when the video playing starts to be slow because of that.
I think we will profit from some longer videos which we have in https://merge-ci.openmicroscopy.org/web/webclient/?show=dataset-25919 where @jburel is planning to add the ROIs (tracks and points or elipses) to have a good judgement.

will-moore · 2020-09-24T10:31:59Z

The video doesn't start to slow because of keeping Shapes in hand.
Just trying again locally using commit 7096856: keeping shapes in hand has some other benefits...

If you move back and forth between 2 or more planes, you don't have a delay to load shapes each time and you see them instantly on the viewer (actually, I still do the request - could probably avoid that).
Also, if I get rid of shapes that are not on current plane then we lose selection state: E.g select a shape, move to next Z/T then back again. The same shape should still be selected, but we lose the selection if we 'only' have shapes for the current plane loaded each time.
The main reason to load shapes per-plane is to make the loading faster, not because we only want to have the shapes for one plane loaded at at time.

So, if I can fix the other issue reported above, hopefully this will improve the experience. I think actually there's only one:

When you uncheck 'Show' on an ROI, then expand the ROI, all the newly loaded shapes should not be visible.

will-moore · 2020-09-24T13:54:37Z

OK, so that commit ee00f4e should fix "When you uncheck 'Show' on an ROI, then expand the ROI, all the newly loaded shapes should not be visible."
I also avoid re-loading ROIs for a plane if we've already loaded them. This means we don't have "Loading Regions..." if we have already loaded them.

pwalczysko · 2020-09-24T14:09:50Z

@will-moore studying https://merge-ci.openmicroscopy.org/web/webclient/img_detail/144215/?dataset=25919 (thanks @jburel for the ROIs), I can see

massive improvement, now I can watch single cells and judge whether or not that particular track is valid
the biggest drawback (comparably minor to Ad. 1): As the shapes are ordered inside the ROIs by time, the low-times shapes are at the bottom, the high-time shapes are at the top. This seems unnatural to me (as I have to always start at the top, because have to expand the ROI to show the shape). Thus, I have to scroll towareds the bottom of the table to find the first shape and click on it (to have a starting point of my video play, where the cell is first detached) and then play the video. The re-ordering by clicking onto the "T" in the header of the column seems to get ignoerd, instead, it will close the expanded ROI on me if I click the T

But as indicated, already now it is a massive improvement imho

pwalczysko · 2020-09-25T11:06:36Z

OK, so that commit ee00f4e should fix "When you uncheck 'Show' on an ROI, then expand the ROI, all the newly loaded shapes should not be visible."
I also avoid re-loading ROIs for a plane if we've already loaded them. This means we don't have "Loading Regions..." if we have already loaded them.

Both points above work as described. The unchecking of the ROI indeed removes the Shapes from display even as you are reloading the ROIs and shapes anew after a new plane was navigated. The planes which are re-visited do not get the "loading ROIs" message and wait.

joshmoore

Thoughts from a first go through, also:

Looking at calls to get_rois_by_plane which take ~4s, am I right that this could best be next optimized by having a get_rois_by_tile?
After running a movie (I think with no ROI selected) under inspection in Chrome, then turning off the movie, then opening/closing/selecting ROIs and shapes, I had some odd behavior, almost like callbacks triggering after the fact.

joshmoore · 2020-09-25T12:02:27Z

plugin/omero_iviewer/views.py

    # Modify query to only select count() and NOT paginate
    query = get_query_for_rois_by_plane(the_z, the_t, z_end, t_end)
-    query = query.replace("distinct(roi.id)", "count(distinct roi.id)")
+    query = query.replace("select roi", "select count(distinct roi.id)")


This could be an argument to get_query_...().

Done in 8fa122c

plugin/omero_iviewer/views.py

will-moore · 2020-09-28T10:49:26Z

Having a get_rois_by_tile() would be a game-changer, particularly for Big images.
The lack of such a query was the reason that #242 was blocked.
That PR also had other usability issues that need discussing - so we may want to take a different path in future, but certainly need the querying first.

will-moore · 2020-09-28T11:58:48Z

@pwalczysko You say "As the shapes are ordered inside the ROIs by time, the low-times shapes are at the bottom, the high-time shapes are at the top." but on https://merge-ci.openmicroscopy.org/web/webclient/img_detail/144215/?dataset=25919 I see to opposite:

Sorting by T column has no effect since this sorts ROIs not shapes within an ROI. I guess it should do both, but the default Shapes order is at least in order of ascending T as I see it?

pwalczysko · 2020-09-28T14:22:49Z

@pwalczysko You say "As the shapes are ordered inside the ROIs by time, the low-times shapes are at the bottom, the high-time shapes are at the top." but on https://merge-ci.openmicroscopy.org/web/webclient/img_detail/144215/?dataset=25919 I see to opposite:

Sorting by T column has no effect since this sorts ROIs not shapes within an ROI. I guess it should do both, but the default Shapes order is at least in order of ascending T as I see it?

No, not in my hands. See screenshot below from https://merge-ci.openmicroscopy.org/web/webclient/img_detail/144215/?dataset=25919

But this is the same image you are pointing me to. Not sure why do we see different behaviour ?

will-moore · 2020-09-29T11:36:09Z

For some reason, Firefox showed it in reverse. Not sure exactly why, but I think I fixed it.

pwalczysko · 2020-09-30T09:41:54Z

1f07bf7 works as expected now in Firefox, it indeed reversed the order of shapes

will-moore · 2020-10-01T08:29:43Z

@jburel I think Petr is happy with this now, but you've not had a look in a while. Could you give it a go and see what you think needs doing? Thanks.

When ROIs paginated, don't load shapes until ROI expanded

1b69fcf

mtbc reviewed Sep 7, 2020

View reviewed changes

plugin/omero_iviewer/views.py Outdated Show resolved Hide resolved

will-moore added 4 commits September 7, 2020 17:14

flake8 fixes

f12ac2f

Merge remote-tracking branch 'origin/master' into lazy_load_shapes

c9df069

regions_info Regions() class handles showChanged

b15f7d3

Don't need to use BindingEngine in the view component

Fix missing this. in Regions() constructor

4bd2dd6

ol3-viewer observes roi.shapes_loaded to add new ROIs to Regions layer

db55ee2

will-moore added 6 commits September 9, 2020 17:23

Rename roi.show to roi.expanded

cf9602c

Rename class Region to Roi, move to new roi.js

262c9cf

Update iviewer_classes.png to match latest iviewer_classes.xml

739b241

Fix iviewer_classes.png link in README

1d3fed5

Add ROI loading/pagination info to docs/README

cf72c18

Add new iviewer_roi_classes diagram and png

2d6b72b

will-moore added 3 commits September 11, 2020 14:29

Preserve hidden status of new shapes when loaded by ROI

3b3d1d9

When selecting shapes, stay on current Z/T plane if possible

a3d8cc1

Fix rois_by_plane shape_count when NO rois on plane

5cee949

will-moore added 3 commits September 15, 2020 17:06

Shapes by plane *added* to viewer without removing old ones

6c9b3c3

Adding more to the docs about how this PR works

95ab106

Fix shape count to fix display of Show checkbox

7096856

pwalczysko reviewed Sep 17, 2020

View reviewed changes

docs/README.md Show resolved Hide resolved

pwalczysko reviewed Sep 17, 2020

View reviewed changes

docs/README.md Show resolved Hide resolved

will-moore added 2 commits September 24, 2020 14:06

New shapes hidden if ROI is hidden

ee00f4e

Clarify that Regions.addFeatures() does not duplicate

39f2c0f

will-moore force-pushed the lazy_load_shapes branch from ef3f2f2 to 39f2c0f Compare September 24, 2020 13:07

Don't re-load ROIs if already loaded for Z/T plane

0fac28b

joshmoore reviewed Sep 25, 2020

View reviewed changes

get_query_for_rois_by_plane() has count param

8fa122c

Fix sorting of Shapes withing ROI on Firefox

1f07bf7

Handle no regions_info when loading e.g. ?dataset=1

fdb29a7

pwalczysko mentioned this pull request Mar 22, 2021

ROI popup dialog fix #374

Merged

will-moore closed this Mar 22, 2021

jburel mentioned this pull request Aug 18, 2021

omero table link to shape ome/omero-web#310

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load shapes #351

Lazy load shapes #351

will-moore commented Sep 7, 2020 •

edited

Loading

jburel commented Sep 8, 2020

will-moore commented Sep 8, 2020

jburel commented Sep 8, 2020

will-moore commented Sep 9, 2020

jburel commented Sep 10, 2020 •

edited

Loading

jburel commented Sep 10, 2020

will-moore commented Sep 10, 2020

pwalczysko commented Sep 15, 2020

will-moore commented Sep 15, 2020

jburel commented Sep 17, 2020

jburel commented Sep 17, 2020

jburel commented Sep 17, 2020 •

edited

Loading

pwalczysko commented Sep 17, 2020 •

edited

Loading

will-moore commented Sep 17, 2020

will-moore commented Sep 23, 2020

pwalczysko commented Sep 24, 2020

will-moore commented Sep 24, 2020

will-moore commented Sep 24, 2020

pwalczysko commented Sep 24, 2020

pwalczysko commented Sep 25, 2020

joshmoore left a comment

joshmoore Sep 25, 2020

will-moore Sep 28, 2020

will-moore commented Sep 28, 2020

will-moore commented Sep 28, 2020

pwalczysko commented Sep 28, 2020

will-moore commented Sep 29, 2020

pwalczysko commented Sep 30, 2020

will-moore commented Oct 1, 2020

Lazy load shapes #351

Lazy load shapes #351

Conversation

will-moore commented Sep 7, 2020 • edited Loading

jburel commented Sep 8, 2020

will-moore commented Sep 8, 2020

jburel commented Sep 8, 2020

will-moore commented Sep 9, 2020

jburel commented Sep 10, 2020 • edited Loading

jburel commented Sep 10, 2020

will-moore commented Sep 10, 2020

pwalczysko commented Sep 15, 2020

will-moore commented Sep 15, 2020

jburel commented Sep 17, 2020

jburel commented Sep 17, 2020

jburel commented Sep 17, 2020 • edited Loading

pwalczysko commented Sep 17, 2020 • edited Loading

will-moore commented Sep 17, 2020

will-moore commented Sep 23, 2020

pwalczysko commented Sep 24, 2020

will-moore commented Sep 24, 2020

will-moore commented Sep 24, 2020

pwalczysko commented Sep 24, 2020

pwalczysko commented Sep 25, 2020

joshmoore left a comment

Choose a reason for hiding this comment

joshmoore Sep 25, 2020

Choose a reason for hiding this comment

will-moore Sep 28, 2020

Choose a reason for hiding this comment

will-moore commented Sep 28, 2020

will-moore commented Sep 28, 2020

pwalczysko commented Sep 28, 2020

will-moore commented Sep 29, 2020

pwalczysko commented Sep 30, 2020

will-moore commented Oct 1, 2020

will-moore commented Sep 7, 2020 •

edited

Loading

jburel commented Sep 10, 2020 •

edited

Loading

jburel commented Sep 17, 2020 •

edited

Loading

pwalczysko commented Sep 17, 2020 •

edited

Loading