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

add transparency slider for human annotation & english/spanish support for controls window #140

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Felipegalind0
Copy link
Contributor

transparency_root_painter

@Abe404
Copy link
Owner

Abe404 commented May 29, 2024

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 :)

@Felipegalind0
Copy link
Contributor Author

Felipegalind0 commented May 29, 2024

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.

@Felipegalind0
Copy link
Contributor Author

also just added Spanish language support to so my uncle can read it

@Felipegalind0 Felipegalind0 changed the title add transparency slider for human annotation add transparency slider for human annotation & english/spanish support for controls window May 29, 2024
@Abe404
Copy link
Owner

Abe404 commented May 30, 2024

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: #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

@Felipegalind0
Copy link
Contributor Author

Felipegalind0 commented May 30, 2024

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:

Screenshot 2024-05-30 at 12 37 24 PM

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?

@Felipegalind0
Copy link
Contributor Author

This would probably be faster right?

im_utils.py:

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)

@Abe404
Copy link
Owner

Abe404 commented May 30, 2024

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

@Felipegalind0
Copy link
Contributor Author

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?

@Felipegalind0
Copy link
Contributor Author

would making a new branch work?

@Abe404
Copy link
Owner

Abe404 commented May 31, 2024 via email

@Abe404
Copy link
Owner

Abe404 commented Jun 10, 2024

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.

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.

2 participants