Open Felipegalind0 opened 6 months ago
Thanks for suggesting this. It looks like it could be useful but I'm not 100% convinced it's the right move yet so would like to discuss a little.
Could you explain a little more behind the background and motivation for this feature? Is there a story behind it?
What is the problem that it solves? Is it born out of a real world use case or just something you are experimenting with?
I think it might be a really good idea but I am not 100% sure about the proposed design and functionality yet, and am also wondering if the UI layout might need some more tweaks.
I'm concerned that it adds more complexity to the interface and also reduces the area of the image that is visible (by pushing up the UI space). It's a minor concern but something to keep in mind and can be more of a concern for users with smaller screens who want multiple windows open at the same time.
I could also try to reach out to some users at some point to see if this is something they want/need.
My gut feeling is that it's a good idea but I want to dig a little further before pushing ahead :)
You cannot draw on the annotation if it is not enabled, therefore you have to disable annotation so you can see the image, and then re-enable it so you can draw
I totally agree that the UI needs to be designed better so it does not steal space from the image, but I find it very frustrating to have to spam the 'A' key all the time.
For now, I think this crappy implementation is better than 100% transparency all of the time
This was not my idea, it was my uncle's. He is 62, and unlike us, young folks, his persistence of vision is not as good, and he was not enjoying pressing 'A' all of the time. He was really struggling to see what he was painting over.
also just added Spanish language support to so my uncle can read it
Thanks! I think you make some good points and I now feel I better understand the issue.
I totally agree that the UI needs to be designed better so it does not steal space from the image, but I find it very frustrating to have to spam the 'A' key all the time.
I feel that this is also the case with the segmentation. i.e users need to frequently hide and show the segmentation (spamming the S key). Do you feel there should be a separate slider for segmentation opacity or the one slider should work for both annotation and segmentation opacity?
For now, I think this crappy implementation is better than 100% transparency all of the time
I get your point, but maybe with a few tweaks we might get something we are happy with.
Do you think this might work better / look cleaner in terms of UI if we had it as a separate window? I feel that it might.
I mean a separate window like what there is for the different measurements options.
For example from the main view menu (or perhaps 'Options' menu would be a better fit, as there's already a contrast enhance feature there) there could be an option called 'Adjust transparency' and then this could open a small widget/window with separate sliders for segmentation and annotation (if you agree they should be separate? Im not sure).
That way users who want to have these sliders shown constantly can have them open in a separate window they can place wherever they like. Perhaps it's also not something that needs to be constantly adjusted, so then they can be hidden away once users have a setting they are happy with. Maybe having it as a separate window can allow us to postpone the decision of how best to integrate it into the main UI (that's constantly visible) if we eventually did decide to do that.
My thoughts with suggesting having this as a standalone window/widget is that it could solve the potential issue with the UI in the main window getting cluttered, that users don't have an option to hide. We could also add a keyboard shortcut to show/hide the transparency widget/window that could then be mentioned in your new multilingual help window.
BTW: This is slightly related to issue: https://github.com/Abe404/root_painter/issues/24 which is about having an outline view (see Figure 2b in https://arxiv.org/pdf/2106.11942 for an example). The outline view would show the outline of the segmentation with the annotation applied, i.e the 'corrected' segmentation. This view might solve many of the issues you are trying to solve with these transparency sliders, but it also has pros and cons. I'm open to exploring both.
Also, regarding your uncle, for him to get the updates do you need me to produce a new release? Or is he able to directly run from source (so he can immediately test out your features directly from your branch).
Thanks so much for your contributions, clarifications and the discussion.
Kind regards, Abraham
After I said
I totally agree that the UI needs to be designed better so it does not steal space from the image
I did some thinking, and came to the conclusion that the easiest way to prevent the UI from getting of the way of the user when screen real state is limited is to remove the gray background on the bottom_bar and just make it transparent.
If we made it transparent, and you zoom out enough, the UI will be still have a gray background, so I do not think there are any readability concerns
It sounded like a simple solution, I tried for 1h yesterday, but could not figure out how to do so and gave up.
I'll make the slider change the transparency for both the Human Annotation and AI Segmentation, KISS. I also think it would be a good idea to make the slider smaller, by placing to the right of the "Brush Size" label
Do you think it is a good idea to make the bottom_bar transparent? Any idea how to do so?
About the outline:
I think it would be awesome to add an outline, but that complicates things because we'd have to use OpenCV to compute a binary mask image, and cv2.findContours.
I might try to adapt this old code from here:
compute a binary mask image:
# Load the mask image
mask_img = cv2.imread(mask_img_path, cv2.IMREAD_COLOR)
# Convert the mask image to grayscale
mask_img_gray = cv2.cvtColor(mask_img, cv2.COLOR_BGR2GRAY)
# Threshold the mask image to get a binary image
_, mask_img_bin = cv2.threshold(mask_img_gray, 127, 255, cv2.THRESH_BINARY)
# Define the kernel size
kernel_size = 5
# Define the kernel
kernel = cv2.getStructuringElement(cv2.MORPH_RECT, (kernel_size, kernel_size))
# Apply morphological opening to the binary mask image
mask_img_opened = cv2.morphologyEx(mask_img_bin, cv2.MORPH_OPEN, kernel)
Find Contours
# Find the contours in the binary mask image
contours, _ = cv2.findContours(largest_comp, cv2.RETR_EXTERNAL, cv2.CHAIN_APPROX_SIMPLE)
can you think of a better way of doing it?
This would probably be faster right?
def get_outline_pixmap(seg_slice, annot_slice):
assert seg_slice.shape == annot_slice[0].shape, (
'get_outline_pixmap: '
f'seg_slice shape {seg_slice.shape} should match '
f' annot_slice shape {annot_slice.shape}')
seg_map = (seg_slice > 0).astype(int)
annot_plus = (annot_slice[1] > 0).astype(int)
annot_minus = (annot_slice[0] > 0).astype(int)
# remove anything where seg is less than 0 as this is outside of the box
seg_minus = (seg_slice < 0).astype(int)
mask = ((((seg_map + annot_plus) - annot_minus) - seg_minus) > 0)
dilated = binary_dilation(mask)
outline = dilated.astype(int) - mask.astype(int)
np_rgb = np.zeros((outline.shape[0], outline.shape[1], 4))
np_rgb[outline > 0] = [255, 255, 0, 180]
q_image = qimage2ndarray.array2qimage(np_rgb)
return QtGui.QPixmap.fromImage(q_image)
Yes, I think for the outline code it makes sense to take the implementation from RootPainter3D and modify it as required.
btw, just to simplify the review process, I think in the future it could be easier to split pull requests up so there’s one for each feature/modification.
Either way, I’m grateful for your contributions
I'd prefer to make separate pulls too, please do not merge this one, I just found I bug with my last commit.
I have been committing to my fork, and I did not want my commits to be added to this pull request, GitHub did that automatically, and I do not know how to disable that, do you know how to disable that?
would making a new branch work?
I think putting each in a separate branch is the best solution.
On Fri, 31 May 2024 at 00.31, Felipe Galindo @.***> wrote:
would making a new branch work?
— Reply to this email directly, view it on GitHub https://github.com/Abe404/root_painter/pull/140#issuecomment-2140954717, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAC33Z3ELOKRWC3MCBH4ZWTZE6SD5AVCNFSM6AAAAABIPWAVY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBQHE2TINZRG4 . You are receiving this because you commented.Message ID: @.***>
Could you perhaps split this up a little? I'm thinking it would be nice to perhaps review and accept the Spanish language support for your help screen as a separate pull request? I get the impression those changes will be easier to accept without as much discussion.