[vim/vim] popup image: fix layering of overlapping images, avoid kitty retransmission on redraw (PR #20474)
mattn
Two fixes for popup window images.
Re-emitting every popup image at the end of each redraw caused two problems: a lower zindex image was painted over a higher popup's cells and image where they overlap (the walk ignored zindex), and with the kitty backend the whole image sequence was retransmitted on every redraw, making cursor movement laggy (for a 1200x1200 RGBA image that is about 7.7MB of base64 per redraw).
The end-of-redraw pass is now GUI-only; in terminal mode the zindex-ordered emit in update_popups() already produces the correct layering, and after emitting a sixel image the cells covered by a higher zindex popup are invalidated in ScreenLines so that popup rewrites them on the same walk. Kitty placements persist on the terminal and are now created with z=zindex, so while a placement is still current the retransmission is skipped entirely: with the reporter's scenario, cursor movement now sends no image data at all (placements are re-sent only after a screen clear or when the image/position actually changes).
Fixes #20460
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20474
Commit Summary
- def3118 popup image: fix layering of overlapping images, stop kitty retransmission
File Changes
(8 files)
- M src/drawscreen.c (7)
- M src/kitty.c (13)
- M src/popupwin.c (118)
- M src/proto/kitty.pro (2)
- M src/proto/popupwin.pro (1)
- M src/screen.c (5)
- M src/structs.h (4)
- M src/vim.h (3)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20474)oh and please re-generate the proto files. The CI failed because of a change in the proto files
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20474)Fixed in 967c5ac, all points addressed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#20474)This fixes the performance when the image is not moving (screen not being scrolled). But as soon the screen scrolls and part of the image comes off screen, it starts lagging again. I think this is because the current logic emits all the image data on every redraw if I am looking at the code correctly.
According to this: https://sw.kovidgoyal.net/kitty/graphics-protocol/#display-images-on-screen. It seems like you can save the image to the terminal without displaying it, then display it multiple times (e.g. delete old placement, create new placement at new position) without retransmitting all the data. @mattn what do you think?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20474)Thanks, you're right. This PR removes the retransmission on cursor movement, but scrolling still re-encodes because the crop changes. The proper fix is the transmit/place split you mention (a=t to store the image once, a=p with a source rectangle to draw the cropped region without resending the pixels). Since that's a larger change to the kitty encoder, I'd like to land this PR first and handle it in a follow-up.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Christian Brabandt
chrisbra left a comment (vim/vim#20474)Thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Foxe Chen
64-bitman left a comment (vim/vim#20474)Thanks, you're right. This PR removes the retransmission on cursor movement, but scrolling still re-encodes because the crop changes. The proper fix is the transmit/place split you mention (
a=tto store the image once,a=pwith a source rectangle to draw the cropped region without resending the pixels). Since that's a larger change to the kitty encoder, I'd like to land this PR first and handle it in a follow-up.
@mattn Are you still working on this change? If not, I can try working on it. Thanks
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
mattn
mattn left a comment (vim/vim#20474)@64-bitman yes! please
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.