ENH: collapsible group box

[GuillaumeFavelier]

This PR adds support for collapsible group boxes for the notebook 3d backend. Here is an example with coreg:

main	PR

I know the main render view still appears stretched, there might be way to fix its size somehow

It's an item of #8833

[GuillaumeFavelier]

And in video:

output.mp4

[larsoner]

I know the main render view still appears stretched, there might be way to fix its size somehow

In the end it's just an image display, right? It seems like there must be a way in ipyvtklink (or whatever it depends on) to make the image's aspect ratio be respected rather than having it get stretched. It seems like some CSS like object-fit might be enough to do it

[larsoner]

...I would try adding this property directly in Chrome's inspect mode to see if it works, and if so, then dig into which library would need to have this option added, then add it and make an upstream PR... and for now if we can add our own CSS inline or in a little cell header (seems like it should be possible?) then we can add it ourselves, too -- that way people don't have to upgrade ipyvtklink or whatever to get the benefit.

[GuillaumeFavelier]

Thank you for the great suggestions @larsoner !

According to ipywidgets' doc, using object_fit='contain' should preserve the aspect ratio:

'contain': Fit the image in its content box while preserving the aspect ratio. If any of the container is not covered by the image, the background of the container is displayed. The content box is the size of the container if the container is smaller than the image, or the size of the image if the container is larger.

I made a comparative table to showcase the different options with collapsed groupbox or not:

object_fit\collapsed	True	False
'contain'
'cover'
'fill'
'none'

[GuillaumeFavelier]

No need for an upstream change, the object_fit property of ipyvtklink's viewer is directly accessible (c.f. 6f44268)

[GuillaumeFavelier]

I chose object_fit='contain' because although 'cover' also preserves the aspect ratio of the image, it will crop it if needed (visible in the table above). From object_fit's doc:

'cover': Fill the content box completely while preserving the aspect ratio of the image, cropping the image if necessary.

I realize this might be controversial because what we lose during cropping is basically "background", for the particular case of CoregistrationUI, but for Brain, it might be a different story (they both use the same viewer configuration).

[GuillaumeFavelier]

What do you think @agramfort, @larsoner?

[agramfort]

I am interested in a live demo :)

…

Message ID: ***@***.***>

[larsoner]

Contain is the right choice I think

[GuillaumeFavelier]

This is ready to go from my end

[GuillaumeFavelier]

During a video call with @agramfort, we discovered a bug related to picking which is introduced in this PR. I have to debug before merge.

[larsoner]

During a video call with @agramfort, we discovered a bug related to picking which is introduced in this PR. I have to debug before merge.

My guess is that it's an ipyvtklink bug where they assume that the click in the box containing the image is in the corresponding part of the stretched image regardless of the object_fit mode. So probably an upstream fix is necessary?

[GuillaumeFavelier]

My guess is that it's an ipyvtklink bug where they assume that the click in the box containing the image is in the corresponding part of the stretched image regardless of the object_fit mode.

Hm... Assuming that the picking with the Pick() function in itself is alright, it could be related to the coordinates returned by GetEventPosition() which in turn could use an obsolete "window" size.

I'll check the size returned by the viewer both in main and in here.

So probably an upstream fix is necessary?

Well, yes, if I cannot pinpoint the bug or find no way to fix it.

[GuillaumeFavelier]

I will share what I have learned:

ipyvtklink relies on ipyevents to get the mouse event locations. Then it does a little bit of offset computation to adjust the given coordinates from the canvas size (which depends on the notebook) to the render window size (which is fixed by the Renderer at init).

What happens when all the group boxes are folded is that the main 3d viewer in the middle is the widget with the highest height value, defining how stretched the container widget (HBox) is. At this point, the values given by ipyevents for boundingRectHeight and boundingRectTop used for offset calculation are correct because the image and its bounding box have the same size.

So the issue with object_fit='contain' arises when the group boxes are unfolded, the bounding box of the image is stretched with the accordion whereas the image is not (that's the point, we want to keep the aspect ratio). And now, the sizes sent by ipyevents will not match anymore.

It is possible to mitigate by using object_position='top', this fixes the value of boundingRectTop but boundingRectHeight is still incorrect so all the accordions must be folded to have valid pick positions.

I thought of 2 solutions to deal with this:

1. Prevent the image's container to stretch.
2. Adjust the event position given by the picker

I did not manage to make 1. work by playing around with layout but it's possible that I missed something and 2. is very hacky because I did not find any convenient way to get the real size of the canvas once it is displayed.

[larsoner]

This sounds like an upstream ipyvtklink bug, I would open an issue there and see if people have ideas

[GuillaumeFavelier]

Meanwhile, 5014f41 is the kind of hack I have to use to make it work with 2.

[GuillaumeFavelier]

@larsoner I opened Kitware/ipyvtklink#35

[GuillaumeFavelier]

I see a few options for the PR:

• merging with object_fit and the hack (honestly, I would feel better if someone else could try it before)
• merging without object_fit (I can revert my latest changes)
• do not merge until How to get the correct event position when object_fit='contain'? Kitware/ipyvtklink#35 is resolved

[larsoner]

For now I would say get rid of object_fit in this PR, and let's see if ipyvtklink can make an upstream fix soon.

@GuillaumeFavelier could you see if you can change their computation in the "a little bit of offset computation" code to fix the bug at their end? It might move things forword (much) faster than waiting for them to fix it.

[GuillaumeFavelier]

could you see if you can change their computation in the "a little bit of offset computation" code to fix the bug at their end?

I'll see what I can do but I think it might be better to handle it with ipyevents directly.

I updated #8833 with:

• add support for object_fit='contain' and object_position='top' (ipywidgets)

[GuillaumeFavelier]

This is ready to merge from my end @agramfort, @larsoner

[larsoner]

I'll see what I can do but I think it might be better to handle it with ipyevents directly.

Could be, want to open an issue with them, too? They should know if it's in scope for them or not

[larsoner]

(if it's a fairly trivial fix, sometimes I open a PR-as-issue to just fix the problem, and ask if they think it's acceptable)

[larsoner]

Thanks @GuillaumeFavelier ! On to the next blocker for 1.0, if any?

[GuillaumeFavelier]

I pick from #8833 really. The 1.0 section is done so I'm working on bonus territory and bug fixes.
Feel free to let me know if there is something in there which has higher priority in your opinion.

[GuillaumeFavelier]

For now, I'm on #10305 and I plan to tackle next:

• multiple threads should block window exit

since it should be functionally similar

[larsoner]

I would say "show a dialog to save if trans is not saved" and "multiple threads should block window exit" make the most sense next