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

Fill rois #577

Merged
merged 31 commits into from
Nov 20, 2024
Merged

Fill rois #577

merged 31 commits into from
Nov 20, 2024

Conversation

Rdornier
Copy link
Contributor

@Rdornier Rdornier commented Jul 9, 2024

Hello,

This PR should fiy the issue #485 about ROI filling.
If you load ROIs from OMERO, the fill-color is read correctly but not the transparency. I wasn't able to get it from the imported json object. So, the current transparency is set to the loaded ROI.

Rémy.

@will-moore
Copy link
Member

You shouldn't commit any files under omero_figure/static/figure/... - They should be ignored by .gitignore.

The following change should give you FillOpacity (and StrokeOpacity) from Shapes loaded from OMERO:

diff --git a/src/js/models/roi_model.js b/src/js/models/roi_model.js
index eb8411b4..f8fe3b54 100644
--- a/src/js/models/roi_model.js
+++ b/src/js/models/roi_model.js
@@ -14,6 +14,12 @@ var ShapeModel = Backbone.Model.extend({
             intAsHex = ("00000000" + intAsHex).slice(-8);
             return '#' + intAsHex.substring(0,6);
         }
+        var rgbint_to_opacity = function(signed_integer) {
+            if (signed_integer < 0) signed_integer = signed_integer >>> 0;
+            var intAsHex = signed_integer.toString(16);
+            intAsHex = ("00000000" + intAsHex).slice(-2);
+            return parseInt(intAsHex, 16) / 255;
+        }
         shape.id = shape['@id'];
         shape.type = shape['@type'].split('#')[1];
         delete shape['@id']
@@ -27,6 +33,9 @@ var ShapeModel = Backbone.Model.extend({
         _.each(["StrokeColor", "FillColor", ], function(attr) {
             if (shape[attr] !== undefined) {
                 shape[lowerFirst(attr)] = rgbint_to_css(shape[attr]);
+                // strokeColor -> strokeOpacity, fillColor -> fillOpacity
+                let opacityAttr = lowerFirst(attr).replace("Color", "Opacity")
+                shape[opacityAttr] = rgbint_to_opacity(shape[attr]);
                 delete shape[attr];
             }
         });

However, the opacity needs formatting to e.g. 2 decimal places to avoid this layout issue:

Screenshot 2024-07-10 at 22 43 03

Even without that, it would be nice to avoid the Load ROIs button wrapping:
Screenshot 2024-07-10 at 22 46 41

(e.g. move it to the top of the right column)

@will-moore
Copy link
Member

NB: I'm away for holidays after tomorrow so it may be a while before I can look at this again...

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#118. See the console output for more details.
Possible conflicts:

--conflicts

@will-moore
Copy link
Member

I've removed the include label to avoid the "Conflicting PR" messages every day. #549 isn't quite ready to merge just yet so I'm afraid we'll have to wait a bit.

@Rdornier
Copy link
Contributor Author

Hi @will-moore

Thanks for the feedback and the suggestion

You shouldn't commit any files under omero_figure/static/figure/... - They should be ignored by .gitignore.

It has been included in the gitignore

I added the rounding with 2 decimales

NB: I'm away for holidays after tomorrow so it may be a while before I can look at this again...

No worries, enjoy your holidays !

I've removed the include label to avoid the "Conflicting PR" messages every day.

OK

Rémy

@will-moore
Copy link
Member

@Rdornier This is looking pretty good now. I haven't tested the PDF/Tiff export yet. I seem to remember there were some issues there, which is why I removed the fillOpacity code from the ShapeEditor code when I ported it over. I'll give that a go anyway...
I had a go at fixing the toolbar wrapping above. See last commit at https://github.com/will-moore/figure/commits/fill-rois/ which gives the toolbar the whole top row of the dialog.

@Rdornier
Copy link
Contributor Author

Hi @will-moore

I haven't tested the PDF/Tiff export yet. I seem to remember there were some issues there

True, I have to check as well
I tried to build the app, as described in the readme but it gives me this output

$ npm run build

 >[email protected] build
 >vite build --emptyOutDir && ./deploy_build.sh

vite v3.2.10 building for production...
transforming...
✓ 139 modules transformed.
rendering chunks...
../omero_figure/static/omero_figure/assets/luts_10.863786e7.png   18.12 KiB
../omero_figure/static/omero_figure/index.html                    63.62 KiB
../omero_figure/static/omero_figure/assets/index.3fe885b4.css     251.65 KiB / gzip: 42.17 KiB
../omero_figure/static/omero_figure/assets/index.05660f14.js      659.28 KiB / gzip: 206.62 KiB
../omero_figure/static/omero_figure/assets/index.05660f14.js.map  2277.14 KiB
'.' is not recognized as an internal or external command,
operable program or batch file.

I don't know if this is expected, probably not. But then, how can I use this output to test the save/export ?

I had a go at fixing the toolbar wrapping above. See last commit at https://github.com/will-moore/figure/commits/fill-rois/ which gives the toolbar the whole top row of the dialog.

Ok

@Rdornier
Copy link
Contributor Author

I've updated the Figure_to_pdf script acording to what I understood from the code and from what was missing to fit with the new feature.
I didn't manage yet to make and use the figure build, so, I didn't test if the changes worked or not.

@will-moore
Copy link
Member

This is what I see:

$ npm run build

> [email protected] build
> vite build --emptyOutDir && ./deploy_build.sh

vite v3.2.10 building for production...
✓ 139 modules transformed.
../omero_figure/static/omero_figure/assets/luts_10.863786e7.png   18.12 KiB
../omero_figure/static/omero_figure/index.html                    61.77 KiB
../omero_figure/static/omero_figure/assets/index.676c7de2.css     251.38 KiB / gzip: 42.09 KiB
../omero_figure/static/omero_figure/assets/index.c141c1a9.js      656.93 KiB / gzip: 206.49 KiB
../omero_figure/static/omero_figure/assets/index.c141c1a9.js.map  2418.04 KiB

(!) Some chunks are larger than 500 KiB after minification. Consider:
- Using dynamic import() to code-split the application
- Use build.rollupOptions.output.manualChunks to improve chunking: https://rollupjs.org/guide/en/#outputmanualchunks
- Adjust chunk size limit for this warning via build.chunkSizeWarningLimit.
Deploying built resources to plugin directory...

It looks like the JS build worked OK. I guess you see the various assets under ../omero_figure/static/omero_figure/assets/ ?
Can you just try running the ./deploy_build.sh script manually to copy those assets into the correct place?

$ ./deploy_build.sh
Deploying built resources to plugin directory...

@Rdornier
Copy link
Contributor Author

I guess you see the various assets under ../omero_figure/static/omero_figure/assets/ ?

Yes, I do.

Can you just try running the ./deploy_build.sh script manually to copy those assets into the correct place?

It works, thanks. Now, I'm able to fully build and test

@@ -771,7 +777,9 @@ def draw_ellipse(self, shape):
# Draw outer ellipse, then remove inner ellipse with full opacity
rgba = ShapeToPdfExport.get_rgba_int(shape['strokeColor'])
ellipse_draw.ellipse((0, 0, width, height), fill=rgba)
rgba = self.get_rgba_int(shape.get('fillColor', '#00000000'))
r,g,b,a = self.get_rgba_int(shape.get('fillColor', '#00000000'))
Copy link
Member

Choose a reason for hiding this comment

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

This line (and others above) are causing flake8 build failures:

flake8.main.application   MainProcess    652 INFO     Found a total of 99 violations and reported 9
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:651:10: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:651:12: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:651:14: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:709:10: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:709:12: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:709:14: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:780:10: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:780:12: E231 missing whitespace after ','
./omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py:780:14: E231 missing whitespace after ','

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#194. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#195. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#196. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#197. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#198. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#199. See the console output for more details.
Possible conflicts:

--conflicts

@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#200. See the console output for more details.
Possible conflicts:

--conflicts

@will-moore will-moore added this to the 7.2.0 milestone Oct 3, 2024
@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#201. See the console output for more details.
Possible conflicts:

--conflicts

@jburel jburel mentioned this pull request Oct 4, 2024
@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#202. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py

--conflicts

@snoopycrimecop
Copy link
Member

snoopycrimecop commented Oct 6, 2024

Conflicting PR. Removed from build OMERO-plugins-push#203. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • omero_figure/scripts/omero/figure_scripts/Figure_To_Pdf.py

--conflicts Conflict resolved in build OMERO-plugins-push#208. See the console output for more details.

@will-moore
Copy link
Member

Thanks for the updates on this and other PRs. Just a heads up that I'm pretty tied up over the next week or so - Afraid I might not get time to review etc for a bit...

@snoopycrimecop snoopycrimecop mentioned this pull request Oct 11, 2024
@will-moore
Copy link
Member

will-moore commented Oct 22, 2024

Screenshot 2024-10-22 at 16 42 10

A couple of issues:

  • The drop-down for choosing Opacity above looks strange with the 2 decimal places. E.g. 0, 0.1, 0.2, 0.3.. would look better.
  • With Points PR now merged, when I try to select points on the Image in the ROI dialog, I get a console error:
shape_manager.js:679 Uncaught TypeError: l.getFillColor is not a function
   at shape_manager.js:679:27
  • It seems that when I pick a Shape loaded from OMERO (with fill Color rgba 255, 255, 255, 0) and "Add" it to the Figure panel, it seems that the Opacity that is applied to it is the current value of the Fill-Opacity select option. E.g. here the current value is 0.20 and when I add the Ellipse, that's the Opacity it gets (not the 0 opacity loaded from OMERO)
Screenshot 2024-10-22 at 22 36 51
  • It would be nice to fix the toolbar wrapping. Here's a commit where I tried this above - hopefully this will still work: will-moore@7da63a6

@Rdornier
Copy link
Contributor Author

Hi @will-moore

Thanks for your feedback.

The drop-down for choosing Opacity above looks strange with the 2 decimal places. E.g. 0, 0.1, 0.2, 0.3.. would look better.

Ok, I'll correct it

With Points PR now merged, when I try to select points on the Image in the ROI dialog, I get a console error:

I think this is normal, as Point ROI doesn't have yet the methods to get fill etc. I'll add them.

It seems that when I pick a Shape loaded from OMERO (with fill Color rgba 255, 255, 255, 0) and "Add" it to the Figure panel, it seems that the Opacity that is applied to it is the current value of the Fill-Opacity select option. E.g. here the current value is 0.20 and when I add the Ellipse, that's the Opacity it gets (not the 0 opacity loaded from OMERO)

Yes, it is the current behavior. I don't remember why I didn't read the opacity from OMERO. I'll check it.

I'm a bit busy this and next week. So, I'll most probably do the corrections at the beginning of November.

Rémy.

@Rdornier
Copy link
Contributor Author

Rdornier commented Nov 4, 2024

Hi @will-moore ,

I fixed all the points you mentionned above.
Thanks for your careful review !

Rémy.

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

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

Functionality looking good, thanks!

Screenshot 2024-11-11 at 16 25 26

@will-moore will-moore merged commit e0b0d1a into ome:master Nov 20, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants