-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix canvas widget sizing issue #2
Comments
Hi! I see you are using Maybe that should be a feature to add in |
I'm all for this being handled in |
Definitely :) And I think it makes total sense as it's something other people might want as well. |
Awesome! Please let me know when you all can add this - it'd be great to have before I push this out to PyPI |
Mmmh, thinking about it, there is something that annoys me a bit. Like other widgets, the Canvas widget can be displayed multiple times on the page (which we call views), and we cannot force each view to have the same container size. Some views might be in big containers, other views in small containers. That being said, I am not sure it's a good idea to provide an API like a container resize event. It might be better to provide a "responsive" mode where each Canvas view tries its best to fit in its container, respecting the current ratio of the Canvas. What do you think? |
I think that'll work well as long as the ratio of the canvas is maintained |
@martinRenou, do you think it'd be possible/easy to land such a feature in a soon-ish time frame? It'd be awesome to have that "responsive" mode for the canvas so we can count on this widget looking good and being usable across browser widths |
I could possibly add some resize event handlers in the meantime that keep an eye on the canvas' width as it is resized with the browser and then resize the underlying VTK render window, but that can greatly affect the resolution/quality of the rendered image. |
It might not be too much work, although I do this work on my free time so it will of course be slower. I don't have much free week-ends coming. |
Totally fair - I appreciate all the help you're able to give!
Being totally unfamiliar with creating widgets and the ipycanvas source, could you point me to a few places to send me in the right direction? |
For sure, I did not think too much about the actual fix yet. You can find dev install instructions here: https://ipycanvas.readthedocs.io/en/latest/installation.html#development-installation. Note that because you will touch the TypeScript code you will need to rerun There are only two files in ipycanvas that matters here, the Python file and the TypeScript one. One thing to know is that the So I guess the responsive workflow would be to listen to container size changes in views. This can be done by waiting for the |
Awesome, thanks so much for these details! It sounds like everything I'd need to get in the source and start trying things out. I will see if I can find time to try working on this and report back (it may be a little while for me also) |
Note that there is a difference between the size of the Canvas in pixels, and the actual size of the Canvas on the screen. For now I force them to be the same in ipycanvas, but I guess that is what we want to change. We still want to expose the first one to the Python interface, but not the second one, as it would be per-view. |
Don't hesitate to open a draft PR when you have something (even if it does not do anything yet), so that I can quickly guide you |
I might have time to work on this this week-end. Please let me know if you started something, I don't want to discard your work :) |
I haven't had the time to start yet. Thanks for your willingness to help on this! |
I think I might have found an elegant solution that does not require any API changes in ipycanvas. Check it out! https://github.com/martinRenou/ipycanvas/pull/111 |
This has been implemented in https://github.com/martinRenou/ipycanvas/pull/111 and https://github.com/martinRenou/ipycanvas/pull/113. You will soon be able to do: canvas.layout.width = '100%'
canvas.layout.height = 'auto' to get a "responsive" canvas which takes as much space as available while respecting the aspect ratio. |
Can you publish a release for I tried it out from the master branch without noticing any change... I may not have installed it correctly. Or perhaps we need to do the same thing for the |
Will do.
Did you execute the snippet of code I provided? How did you install from master?
Not sure I understand what you are saying. There is no use of ipywidgets.Image in ipycanvas. Do you use a image widget in ipyvtk_simple? |
Yep, since out # Set Canvas to autofill the space
self.layout.width = '100%'
self.layout.height = 'auto'
Just a simple pip install from the master branch: pip install git+git://github.com/martinRenou/ipycanvas@master (within a fresh Docker image)
We use an https://github.com/Kitware/ipyvtk-simple/blob/master/ipyvtk_simple/viewer.py#L57-L61 Do we need to scale the image to fill the canvas dynamically? Is the hard setting of the image size here likely the culprit? |
If you are using Jupyter Notebook that should be enough.
I see. This should be fine. It should work IMO. Also which browser do you use? I'll give ipyvtk_simple a shot on my side before releasing. There might be something I missed. |
Actually you might need to reinstall the Notebook extension: jupyter nbextension install --py --sys-prefix ipycanvas
jupyter nbextension enable --py --sys-prefix ipycanvas Not entirely sure this is needed when pip is ran without |
I just released ipycanvas 0.5.0, it's available on pip, soon available on conda. I tried ipyvtk_simple with 0.5.0 locally and setting |
This works wonderfully! One issue arises if using a width other than 100%: the mouse events registers from |
:S It might actually be difficult to fix this. This is what I do in ipycanvas: https://github.com/martinRenou/ipycanvas/blob/master/src/widget.ts#L404, we compute the mouse coordinates depending on the view size and the canvas size. Which mouse events are you using? Are those not already supported in ipycanvas? Maybe we should support the ones you are using in ipycanvas. |
Turns out this isn't too bad. The event gives you the current size of the canvas as displayed within jupyterlab. You can simply scale the relative mouse offset with: if 'offsetX' in event:
scale_x = self.width/event['boundingRectWidth']
event['offsetX'] = round(event['offsetX']*scale_x)
scale_y = self.height/event['boundingRectHeight']
event['offsetY'] = round(event['offsetY']*scale_y) |
I did not know there were so many information in the mouse event with ipyevents :) |
The widget image should be able to dynamically resize with the browser width and map to the render window. Currently, this isn't working and the image is a fixed width:
The text was updated successfully, but these errors were encountered: