Support zooming and panning in the image viewer#43944
Support zooming and panning in the image viewer#43944MrSubidubi merged 17 commits intozed-industries:mainfrom
Conversation
- add zoom in/out, reset, fit-to-view and zoom-to-actual actions - support scroll-wheel zoom with modifier, click-and-drag panning - renders a zoom percentage overlay. - shortcuts keys for all zoom actions.
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for this! I'll give this a more proper review soonish (got some other things on my list first), but just to give you a few TODOs I just noticed:
- Can we add buttons for the new actions to the toolbar, optimally the right corner? If we do not have icons for some of these, we usually source ours from https://lucide.dev/icons/ (Please only do if you actually have to, we should have quite a few already).
- Whatever the checkered background is doing in your example is.. not optimal. Could you ensure that the checkered background is actually just drawn behind the image (so one can see the transparent parts of the image) and that the squares properly follow the scaling of the image?
Thanks!
- Refactor checkerboard rendering to respect the scaled image size and pan - Add ImageViewToolbarControls that observes the active ImageView and provides zoom in/out, reset
Fixed the checkered background error and this toolbar: 
|
|
weird that the terminal test failed on mac, I am looking into it, but can we re-run the test ? Edit: Yeah the re-run made it pass |
|
The last commit vastly improved the performance, realized we didn't even need to draw it :D |
|
@MrSubidubi Could you review this? |
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for the follow up! A few notes:
- We do need the checkered background for partially transparent images, e.g.
zed/crates/zed/resources/app-icon-preview@2x.png. We do have the outline, but I'd definitely prefer the checkered background, as removing it would feel like a regression. - Dragging does not account for the current zoom factor and can thus get out of sync. Can add a video of this if you'd like, but this basically means that the image does not move as many pixels as I drag on certain zoom levels. IMO this should be 1:1 mapping without any drift.
- The zoom level in the bottom half of the screen can get out of the view. I think we should probably just remove it, because we have the appropiate zoom level now present in the toolbar as well.
That should be it for this round!
Also, quick note, no need to ping me here. I'll get back to you as soon as I can, but this can (sadly) sometimes take some time. Thanks!
I don't know what you mean exactly by the 2nd point, video would be helpful yes! |
25d7f7f to
d176ebf
Compare
|
Sorry for the slight delay in review - here is the video: Bildschirmaufnahme.2025-12-22.um.11.19.44.movThe image should move as far as the cursor here IMO, so the amount of pixels moved by the mouse should be the same for the image itself. Similarly, the sqares in the background should have a constant size, meaning that if I enlarge the images, I also enlarge the squares and vice versa. Does that make sense? |
- compute left/top from container bounds and scaled size and use absolute positioning with pan offsets; - fall back to flex centering when not positioning manually. - only clear dragging state and notify when dragging was active.
So this fixes the offset you are talking about and the squares are magnified with the zoom level. I am although working on optimizing this rendering process, I guess we can do that in the next PR or this itself.. ideas like:
|
|
What's the standard way zed-staff profile render performance/draw calls for GPUI? |
- now using 48.0 * zoom for square size, compute rows/cols from bounds, skip alternate squares - clamp tile size to bounds and paint a single grey color.
|
|
I don't have a build setup to try this myself, but could this be made faster by building a small 2x2 or one-line checkerboard manually and then tiling it? And/or caching the background after creating it once at the correct size and then compositing the image overtop on draw. |
I have tried this yes, my optimization essentially will perform the same, right now I only draw half of the squares with 4x the size, and the one line thing you are talking about, I assume you are talking about PathBuilder/Vector path, GPUI doesn't support that and it also doesn't support the background you mentioned. |
MrSubidubi
left a comment
There was a problem hiding this comment.
Nice, this is so much better already, thank you very much! (And sorry for the delay from my side, was too occupied elsewhere).
I left quite a few comments below, but we should already be close to merging.
Most notably though, we still have two mayor issues as shown in the video here:
Bildschirmaufnahme.2026-01-14.um.18.08.23.mov
As you see,
- there is a brief flicker at the start. This happens I believe because we flex layout once, and I think we should drop the special case for this here. This might be a bit tricky, but we could add a custom element for the image to draw it (would introduce a bit of boilerplate), but I'd like us to get rid of this. If you need more input for this, please hit me up and I'll get into more detail here/look at this myself.
- zooming with the mouse wheel does not take the current positon of the mouse into account and that feels counterintuitive. I think when we resize there, the resize-operation should take the cursor position into account.
Other than that, looks so much better, thank you already! As for profiling, we are currently doing and experimenting a lot with tracy, but there is no official guideline. So far performance has been fine for me, although I fully agree checkered background support would be nice to have here. Feel free to throw a TODO somewhere, but we can also discuss more here.
| .child({ | ||
| canvas( | ||
| move |_, _, _| {}, | ||
| move |_, _, window, _cx| { | ||
| window.on_mouse_event(move |_event: &MouseUpEvent, phase, _window, cx| { | ||
| if phase == DispatchPhase::Bubble { | ||
| if let Some(entity) = entity.upgrade() { | ||
| entity.update(cx, |this, cx| { | ||
| if this.is_dragging { | ||
| this.is_dragging = false; | ||
| this.last_mouse_position = None; | ||
| cx.notify(); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| }, | ||
| ) | ||
| .absolute() | ||
| .size_0() |
There was a problem hiding this comment.
I think that listener can just live within the window without the canvas around it, and we only need to attach it if we are currently actually dragging the image, right?
| focus_handle: FocusHandle, | ||
| zoom_level: f32, | ||
| pan_offset: Point<Pixels>, | ||
| is_dragging: bool, |
There was a problem hiding this comment.
I think we should make is_dragging a method like
fn is_dragging(&self) -> bool {
self.last_mouse_position.is_some()
}, right? We set both simultaneously, so perhaps better to just keep one of these.
|
|
||
| actions!( | ||
| image_viewer, | ||
| [ZoomIn, ZoomOut, ResetZoom, FitToView, ZoomToActualSize] |
There was a problem hiding this comment.
Could we have some generic comments for these actions?
There was a problem hiding this comment.
Added very basic explanations
| project, | ||
| focus_handle: cx.focus_handle(), | ||
| zoom_level: 1.0, | ||
| pan_offset: point(px(0.0), px(0.0)), |
There was a problem hiding this comment.
| pan_offset: point(px(0.0), px(0.0)), | |
| pan_offset: Point::default(), |
Nit, I think that can be done in a few more places and is a bit easier to read IMO.
| if let (Some(bounds), Some((img_width, img_height))) = | ||
| (self.container_bounds, self.image_size) |
There was a problem hiding this comment.
| if let (Some(bounds), Some((img_width, img_height))) = | |
| (self.container_bounds, self.image_size) | |
| if let Some(bounds, (img_width, img_height)) = | |
| self.container_bounds.zip(self.image_size) |
| if let Some(item) = active_pane_item { | ||
| if let Some(image_view) = item.downcast::<ImageView>() { | ||
| self._subscription = Some(cx.observe(&image_view, |_, _, cx| { | ||
| cx.notify(); | ||
| })); | ||
| self.image_view = Some(image_view.downgrade()); | ||
| cx.notify(); | ||
| return ToolbarItemLocation::PrimaryRight; | ||
| } | ||
| } |
| if event.modifiers.control || event.modifiers.platform { | ||
| let delta: f32 = match event.delta { | ||
| ScrollDelta::Pixels(pixels) => pixels.y.into(), | ||
| ScrollDelta::Lines(lines) => lines.y * 20.0, |
There was a problem hiding this comment.
Let's lift the 20.0 here in this method into another constant pleaes
| } else { | ||
| let delta = match event.delta { | ||
| ScrollDelta::Pixels(pixels) => pixels, | ||
| ScrollDelta::Lines(lines) => point(px(lines.x * 20.0), px(lines.y * 20.0)), |
There was a problem hiding this comment.
| ScrollDelta::Lines(lines) => point(px(lines.x * 20.0), px(lines.y * 20.0)), | |
| ScrollDelta::Lines(lines) => lines.map(|delta| px(delta * 20.0)) |
| ScrollDelta::Pixels(pixels) => pixels, | ||
| ScrollDelta::Lines(lines) => point(px(lines.x * 20.0), px(lines.y * 20.0)), | ||
| }; | ||
| self.pan_offset = point(self.pan_offset.x + delta.x, self.pan_offset.y + delta.y); |
There was a problem hiding this comment.
| self.pan_offset = point(self.pan_offset.x + delta.x, self.pan_offset.y + delta.y); | |
| self.pan_offset += delta; |
| let delta = point(event.position.x - last_pos.x, event.position.y - last_pos.y); | ||
| self.pan_offset = point(self.pan_offset.x + delta.x, self.pan_offset.y + delta.y); |
There was a problem hiding this comment.
| let delta = point(event.position.x - last_pos.x, event.position.y - last_pos.y); | |
| self.pan_offset = point(self.pan_offset.x + delta.x, self.pan_offset.y + delta.y); | |
| let delta = event.position - last_pos; | |
| self.pan_offset += delta; |
flicker - added basic info on actions and extracted magic numbers to constants - use intuitive scaling with translation of (1-scale_factor) on the offset - attach global listener only during drag
|
Thank you for the detailed review!
Yeah, this is acceptable performance compared to earlier.
|
There was a problem hiding this comment.
Okay, one mast mid-sized thing and I think we are good, thank you for all your efforts.
I think we need to refactor this a bit so that we have the container bounds of the current frame and not the bounds of the previous frame. Using the information from the previous frame, we
- always do not render an image on the first frame, which causes a brief but noticeable flicker
- always have a small flicker whenever we open other panels/resize because again, sizing calculation is done with sizes from the previous frame.
I think we need to change stuff of this to utilize a small wrapper instead that impls Element. We can then request the layout as we currently do and get the corresponding bounds during prepaint, which we can then utilize and which corresponds to the actual current bounds as opposed to the bounds from the previous frame. Then we should basically be able to use the current render of the image viewer in that prepaint, request the layout slightly delayed there and can prepaint and paint after as needed.
I think
zed/crates/ui/src/utils/with_rem_size.rs
Line 45 in 81ea040
StickyItems (obviously we'd need this to be slightly different), but this would resolve that issue. What do you think?
I wasn't able to get flickering after setting position to left and top. I am not exactly sure by what you mean flickering here. If it's that blink feeling, maybe that could stem from me drawing the background color first and the checkboard pattern rendering before the image. Parallely, I can implement it with the paint, prepaint, request_layout methods. |
I think that is precisely what I wanted to describe and I think we cannot get rid of this unless going for an |
Yeah, this can be added as a todo on GPUI perhaps where the render is parallel for the background and the image. To describe it precisely an ability to set an image background/pattern. |
- Replace canvas-based container bounds tracking with ImageContentElement that wraps a Div and updates bounds during prepaint. - This ensures positioning uses current-frame bounds instead of previous-frame bounds.
|
This should help as the rendering now would use current frame bounds instead of the previous one. |
|
Went ahead and applied the last set of fixes myself - feel free to check these out, we should now always properly layout the image according to the current viewport, no matter what. If there is anything, please let me know, otherwise, I think we are good to go! |
Thank you! I tested it locally and works as expected. I can add gesture support along with adding the following changes in GPUI: Gesture Support GPUI-CE PR as continuation. |
MrSubidubi
left a comment
There was a problem hiding this comment.
Thanks for taking another look!
I think the pinch support sounds interesting, feel free to open a PR for that with the support for the image viewer here and I'll gladly take a look!
Lastly, thank you so so much for bearing with me here and for your perseverance, I for one think (and hope you agree) it was worthwhile and am very happy to merge this now. Thank you so much!
|
Thank you @MostlyKIGuess @MrSubidubi for your work on this. I have been watching this PR eagerly! |
Thanks a ton for the thorough review and patience, really appreciate it. Learned a lot from this one. Looking forward to the pinch-zoom follow-up: #47351 |
Implemented Pan and Zoom on the Image Viewer. Demo: https://github.com/user-attachments/assets/855bafe8-fdc2-4945-9bfb-e48382264806 Closes zed-industries#9584 Release Notes: - Add zoom in/out, reset, fit-to-view and zoom-to-actual actions - Support scroll-wheel zoom with modifier, click-and-drag panning - Renders a zoom percentage overlay. - ImageView Toolbar Added to control zoom in/out and fit view. --------- Co-authored-by: MrSubidubi <finn@zed.dev>
|
Hey @MostlyKIGuess ! Since this landed, we now have checkered backgruound support in our shaders! 🎉 Can I interest you in looking into that? Happy to look at a PR, I'll probably do it myself at some point but currently don't have the time to do it immediately :/ |
Yeah sure, happy to take this up. |
Implemented Pan and Zoom on the Image Viewer.
Demo:
2025-12-02.02-37-27.mp4
Closes #9584
Release Notes: