search

ome / omero-iviewer

An OMERO.web app allowing to view images
https://www.openmicroscopy.org/omero/iviewer/
Other
18 stars 29 forks source link

viewport URL includes z-plane #372

Open will-moore opened 3 years ago

will-moore commented 3 years ago

Fixes #355

To test:

--exclude

pwalczysko commented 3 years ago

Works as expected. Ready to merge fmpov.

jburel commented 3 years ago

I don't think this is working as you expect, also what about T?

will-moore commented 3 years ago

I guess there's a decision to make about whether to store the actual Z index (0-based) or the UI displayed Z index (1-based). I would vote for the 0-based index since it's closer to the truth. e.g. see last commit. Actually I would prefer that the UI was also 0-based (as in napari, vizarr, icy etc) since the off-by-one pain is not worth it and just confuses everyone. There are many situations where you are in a grey area between the UI and the server (e.g. in this case or when exporting ROIs/Shapes in a CSV) where you don't know if the Z/T indices are 0-based or 1-based.

jburel commented 3 years ago

Changing the UI is a breaking change and it needs to be considered carefully: not only in iviewer, but in the doc, video etc.

jburel commented 3 years ago

The feature now works as expected and T is also taken into account

will-moore commented 3 years ago

NB: Although the Z and T were never set in the URL before, it's possible that users were constructing their own URLs with Z and T. In which case, the usage of 0-based indexing here instead of 1-based indexing would be a breaking change. However, this will likely have a much smaller impact to make the change now, instead of after we support the adding of Z and T to the URL (in this PR). And I think that the API should be using 0-based indexing. Thoughts?

jburel commented 3 years ago

The possible problem is that the old viewer (that is still turned on by default) uses 1-based index e.g. https://merge-ci.openmicroscopy.org/web/webgateway/img_detail/1132/?dataset=1051&z=0 will not work

will-moore commented 3 years ago

OK, so either we stick with 1-based index from now on. 👎 Or we also update the old viewer to use 0-based index. 👍 Always best to make a breaking change sooner (while the exposure is less) than later. Votes? cc @joshmoore

sbesson commented 3 years ago

Trying to look into where these conventions might have been documented (or not), I realized that the webgateway render API also uses 0-based indexing e.g. the following calls work

https://idr.openmicroscopy.org/webgateway/render_image/12922202/0/0/ https://idr.openmicroscopy.org/webgateway/render_image/12922202/44/0/ https://idr.openmicroscopy.org/webgateway/render_thumbnail/12922202/?z=0 https://idr.openmicroscopy.org/webgateway/render_thumbnail/12922202/?z=44

but the following calls will return a 404

https://idr.openmicroscopy.org/webgateway/render_image/12922202/45/0/ https://idr.openmicroscopy.org/webgateway/render_thumbnail/12922202/?z=45

Moving forward, unifying on 0-based indexes feels like the most consistent approach across the OMERO.web URLs. For the old viewer, there is definitely a breaking impact. Is this the only place that uses 1-based indexes for Z/T? Is there a way to enfore some compatibility e.g. via key/value pairs or options?

joshmoore commented 3 years ago

It's hot, so I may not be thinking very clearly but some points:

jburel commented 3 years ago

I am for deprecating the old viewer. One of the problems is that the old viewer code is used in the right-hand panel in the main view. So you can explore the image without opening a viewer. deprecating the old viewer will also mean the installing web will come without any viewer. All that need to be balanced carefully

will-moore commented 3 years ago

What does "deprecating the old viewer" mean for users? Remove it (no viewer)? I think a webclient without a viewer would be considered incomplete. So pip install omero-web should include iviewer by default, with all the config enabled already?

will-moore commented 3 years ago

So, should I update the old viewer to use 0-based indexes for z & t. e.g. https://merge-ci.openmicroscopy.org/web/webgateway/img_detail/1132/?dataset=1051&z=0 ??

jburel commented 3 years ago

that could break many people who have bookmarked the link with z=1

will-moore commented 3 years ago

It could. But do we want the possibility of bookmarks to prevent us from making any changes to our API?

An alternative is we use upper case keys for the recent 0-based indices ?Z=0&T=0 so we can distinguish between these and the previous 1-based ?z=1&t=1 URLs?

e.g. Django uses a more explicit naming to distinguish in templates:

forloop.counter | The current iteration of the loop (1-indexed)
forloop.counter0 | The current iteration of the loop (0-indexed)

But then we'd be expected to support 0-based and 1-based indices everywhere. Probably better to make a single breaking change?

jburel commented 3 years ago

What I am arguing is not any change to the API but the path to get there. Before making any change, we should announce that we will change the indexing of the old viewer. We cannot include without given a warning

W

will-moore commented 3 years ago

Ok, so if we are going to announce a breaking change etc, do we also want to make the change in the UI (iviewer and old viewer) too, as I suggested above? Then we are consistent everywhere and don't have to worry about off-by-one errors again?

EDIT: There's also OMERO.figure and iviewer if we want to be consistent everywhere.

will-moore commented 2 years ago

Just reviewing this PR again since the bug-fix that user requested is still not released.

Perhaps the solution is as Josh suggested: Use ?z_index=0 (zero-based index) for creating any new URLs but we maintain support for ?z=1 (1-based index) for existing URLs to avoid a breaking change. We can support this in both the iviewer and webgateway viewer. The only down side is that it's not so concise, but that's a pretty minor issue, and you can't guess from the name that 1 is zero-based and the other is 1-based, but I think that's OK?

joshmoore commented 2 years ago

you can't guess from the name that 1 is zero-based and the other is 1-based, but I think that's OK?

Does z_offset help?

will-moore commented 2 years ago

Or, to indicate how this maps to the model, you could do ?theZ=0

will-moore commented 1 year ago

It would be good to get this fix merged (PR open 2 years now).

I would prefer to adopt ?Z=0&T=0 as zero-based indices going forward. This is the nicest solution and it's worth some extra work to get there.

For old bookmarks of the webgateway viewer, we can handle ?z=1&t=1 and notify users that they should now use zero-based ?Z=0&T=0.

Plan:

OK @jburel ?