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

External images plugin #58

Merged
merged 44 commits into from May 24, 2017
Merged

External images plugin #58

merged 44 commits into from May 24, 2017

Conversation

PiRK
Copy link
Contributor

@PiRK PiRK commented Apr 27, 2017

ExternalImagesPlugin version based on a silx PlotWidget.

The external image is not reduced in size to fit the stack image size (the scales are simply recalculated).

TODO:

  • RGBWidget needs to be modified to accept any image size. Currently an error is raised when two images of different sizes are added.
  • Handle origin different from (0, 0)

@vasole
Copy link
Owner

vasole commented Apr 27, 2017

I think lines 194, 196 and 197 of SilxExternalImagesPlugin.py need to be changed.

@PiRK
Copy link
Contributor Author

PiRK commented Apr 28, 2017

196, 197 fixed. 194 seems to work, but i can replace pop with del, it is equivalent.

@PiRK
Copy link
Contributor Author

PiRK commented Apr 28, 2017

I plan to do ROIStackPlugin in the same PR. I believe there is not much left to do, I can reuse SilxExternalImageWindow and just add the filter and background features.

@vasole
Copy link
Owner

vasole commented Apr 28, 2017

Please, profit to use Roi instead of ROI in new classes and file names in order to be consistent with the CamelCase conventions.

@PiRK
Copy link
Contributor Author

PiRK commented May 22, 2017

Most discussed features are implemented. The widgets still lack Y axis orientation synchronization with the existing PyMca widgets, and I haven't started working on the scatters.

Regarding handling scatter data, in my proof of concepts I did a month ago, I chose to make two separate widgets for image data and scatter data. This made it a bit easier to set up the mask widgets, without needing to have two mask widgets and two sets of setters and getters for the mask in a single widget.

On the other hand, juggling between two separate mutually exclusive widgets in a GUI is also challenging. So I'm not sure which way to go. I would recommend dealing with this question in a separate pull request, as the current PR does not affect the existing PyMca widgets while the scatter mask will probably require to have silx both in the main GUI and in the plugins.

@vasole
Copy link
Owner

vasole commented May 22, 2017

Since the underlying application does not support scatter yet, then just implement the functionality for images. If everything goes well, we'll be able to switch to using silx in the main GUI and in the plugins since the Silx Alternate ROI Window should be able to replace the widgets used in main window GUI. Just do not forget to remove the [WIP] flag when you feel ready.

@PiRK PiRK changed the title [WIP] External images plugin External images plugin May 22, 2017
@vasole
Copy link
Owner

vasole commented May 22, 2017

  • The crop is not working properly. If I crop the image below by first zooming to the actual image region:

beforecrop

I get:

aftercrop

It looks as if the offset (~1.5) would not have been taken into account.

  • It would be desirable to have the copy-to-clipboard silx action in the plots using silx widgets.

@PiRK
Copy link
Contributor Author

PiRK commented May 23, 2017

I just found: it did not take the origin into account and there was an unnecessary rounding of the image limits (expressed in plot coordinates). So basically the crop only worked when the origin was 0 and scale 1 (image size in pixels equal to the stack size).

Now it should work.

@vasole
Copy link
Owner

vasole commented May 23, 2017

Looks good. Can you add the copy-to-clipboard action too?

@PiRK
Copy link
Contributor Author

PiRK commented May 23, 2017

Done.

@vasole
Copy link
Owner

vasole commented May 23, 2017

Bug in the Silx Alternate ROI Window. To reproduce:

  • Open the alternate ROI window,
  • Select a mask
  • Move the slider to select one of the alternative images

@PiRK
Copy link
Contributor Author

PiRK commented May 23, 2017

I have a feeling that it is related to something else that I'm working on with Thomas: silx-kit/silx#836

The funny thing is that if the mask is drawn on the PyMca side and as long as the mask widget is not opened in the silx widget, there is no bug.

@PiRK
Copy link
Contributor Author

PiRK commented May 23, 2017

The bug is fixed. It was caused by removing the image before changing it, when the slider is moved. This caused the mask to be deleted in the silx widget.
Maybe the mask behavior needs to be corrected on the silx side.

@vasole
Copy link
Owner

vasole commented May 23, 2017

There is still a problem. When moving the slider in the Silx Alternative ROI, the existing mask, if any, is lost.

@PiRK
Copy link
Contributor Author

PiRK commented May 24, 2017

I don't see this issue. But I noticed that the mask stays is not drawn in my widget until the mask icon is clicked for the first time.

@vasole
Copy link
Owner

vasole commented May 24, 2017

The problem happens if the ROI widget associated to the Silx window has not been shown.

It seems a problem with some silx optimization at some point. As a workaround, for the time being you can systematically show it docked.

@PiRK
Copy link
Contributor Author

PiRK commented May 24, 2017

I have a simple workaround for the error we saw when the mask widget send a maskUpdated signal with an empty mask at init. I just ignore empty mask (array of size 0) instead of making our widget emit the sigSelectionMaskChanged event.

I also found the reason why the mask seems to be deleted when the slider is activated: the mask is not actually deleted, but the data image is redrawn on top of the mask, on the same layer z=1. This layer is hardcoded in the mask widget in silx. We could modify it to use activeImage.getZLayer() + 1.

When the widget is open, I guess the mask is redrawn after the image changes, so the problem is not visible.

My workaround is to use z=-1 for the background and z=0 for the data.

@vasole
Copy link
Owner

vasole commented May 24, 2017

Well, it seems to work, but I still do not catch how then the external images work.

In my view, both, the external image widget and the mask image widget should be able to accept a "background" image, an image and a mask and I would not like to test for the silx version in the immediate future.

I do not think we are forcing z layer to be positive. Probably working with -1 for the background, 0 for the active image and 1 (hardcoded) for the mask should work. Please, can you give a try at that? If unsuccessful I will accept your pull request as it is.

@PiRK
Copy link
Contributor Author

PiRK commented May 24, 2017

The external images are added as background images, underneath a tranparent stack data image. The mask stays therefore always on top of the visible image.

I already tried what you suggest (z=-1 for bg and z=0 for data), and it works (1046178)

@vasole vasole merged commit 1046178 into vasole:master May 24, 2017
@vasole
Copy link
Owner

vasole commented May 24, 2017

Merged. I would like to get rid of all those warnings when fabIO is not installed.

@PiRK PiRK deleted the external_images_plugin branch October 4, 2017 14:24
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.

None yet

2 participants