Add background image #2419
Add background image #2419
Conversation
So background color transparancy could be changed independenly from window to control background image transparancy
| @@ -16,6 +17,7 @@ pub struct WindowSettings { | |||
| pub touch_deadzone: f32, | |||
| pub touch_drag_timeout: f32, | |||
| pub background_color: String, | |||
| pub background_image: String, | |||
There was a problem hiding this comment.
Should this be in src/renderer/mod.rs in RendererSettings instead? (alongside with background_color)
There was a problem hiding this comment.
I think it makes more sesnse to keep it in window settings as it modifies the window property rather than how the window is being rendered
src/renderer/mod.rs
Outdated
| @@ -148,12 +157,14 @@ impl Renderer { | |||
| self.cursor_renderer.prepare_frame() | |||
| } | |||
|
|
|||
| pub fn draw_frame(&mut self, root_canvas: &Canvas, dt: f32) { | |||
| pub fn draw_frame(&mut self, root_canvas: &Canvas, dt: f32, width: f32, height: f32) { | |||
There was a problem hiding this comment.
Might be able to use Canvas::base_layer_size instead of passing width and height by parameter?
(fwiw you can look up the things available in skia_safe on skia.org, e.g. [SkCanvas::getBaseLayerSize] corresponds to Canvas::base_layer_size. The docs for skia_safe are available directly by running cargo doc --open locally)
There was a problem hiding this comment.
With the latest renderer changes using base_layer_size makes things much more cleaner however it doesn't cover full screen. There is some space left in the bottom that isn't covered by the image.
I don't think it's very critical but it would be nice if image could cover the whole screen. Do you know how this can be achived?
|
Very cool, I was thinking of the same functionality just yesterday. I've a question though, what will happen if Neovide and background images sizes are different? For example:
etc. I wonder that If I make Neovide fullscreen and pass a background image sharing the same screen resolution, no image stretches / paddings will occur? That would be the "ideal" case. |
Thank you for the review @MultisampledNight! Sorry for the wait I just noticed that there is some activity on this pr. I merged latest changes and implimented your suggestions. |
Before it was stretching however in latest commit I made it to keep aspect ratio, fill the screen and center the image and crop the longest side in case if it overflows as suggested here
Yes it would fill the screen without any stretching and padding |
|
(fwiw I am not familiar enough with the neovide codebase anymore to qualitatively review this PR anymore, sorry) |
|
Is there a way to test the artifact without building from sources? |
|
@pidgeon777, I started the CI run, the artifacts should be there once it finishes |
|
@pidgeon777, the artifacts are a bit hard to find currently, we need to improve or at least document it, but you should be able to find them here |
|
I just did a quick test with these settings: vim.g.neovide_transparency = 0.8
-- vim.g.neovide_transparency = 1.0
vim.g.neovide_background_image = "C:\\Users\\<NAME>\\Downloads\\pexels-photo-1483054.jpeg"Background image is now shown but it makes Neovide extremely laggy, even with simple cursor movements in normal mode. Image resolution is 8192 x 5063, 72 DPI, 24 bit |
|
When utilizing an image with a resolution of 1920x1080 as a background, the performance and smoothness of Neovide appear to remain unaffected (but more tests should be done). I would like to inquire about the configuration settings of Neovide, specifically pertaining to the transparency of the background image. Could you provide guidance on how to adjust these settings so that:
Example:
To achieve the above image, I simply moved the Neovide window on the top of a Chrome background image. But it would be great to obtain the second setup by setting the path to the background image in Neovide, and toggle between the amount of image and the neovide background, with the transparency option. |
|
The aforementioned test was conducted using the artifact linked below. This version of the software did not yet include the background image feature. However, it did allow for the adjustment of the Neovide background transparency: |
You can do it with This is also probably why you can't see the background image. Try setting
I tried with 8k image but unfortunatly I wasn't able to reproduce it. Could you send your image that you are testing? |
|
You're right, I didn't know about this new Below the background image causing the lag:
UPDATE: I also zipped it: |
|
I have conducted some tests using the artifacts available at Neovide Actions Run. I collected some observations regarding it:
Given these observations, I have a couple of suggestions for potential improvements: For Issue 1 (Background Image Stretching):
For Issue 2 (Background Image Replication):
|
I think this is quite hard currently. We don't have any support for window specific options in Neovide. And the Neovim gui protocol does not really support it easily either, which means that if we use the Neovim api to do it, things could get out of sync. It's something that we want to add support for in the future though, for example, we should be able to control the shadows, and blur effects per window. |
|
Understand, so for now a simple "hack" to achieve the same effect could be just set |
|
I fixed both stretching and replication bugs
I was able to reproduce it with this image, however I don't know how to fix it. I narrowed the problem down to this call canvas.draw_image_rect(
image,
Some((&src_rect, SrcRectConstraint::Strict)),
window_rect,
paint,
);which draws a background image and it takes 200ms on my pc with the image. I can reduce it to 40ms by unpacking the image into bitmap on loading and using SrcRectConstraint::Fast but it involves unsafe calls and still feels laggy. It probably possible to cache a layer but it will still lag when resizing a window. So I am not sure how else this can be optimized. If anyone have some ideas I am happy to try implimenting them. |
|
@mysty0, are you able to test if a smaller image is fast, I mean the cost should be way less than a ms? In that case, the solution is probably to either scale the image first when loading, or draw in sections loading different parts of the image. |
Yep, for 1080p image I get around 1.5us for background rendering |
|
Does it also support .webp images? |
No, I don't think it supports them |
|
I was able to optimize it to render for less than 1ms for 10k image while text editing. However it still can go up to 15ms when resizing windows but it doesn't really feel that laggy. To further optimize I can probably prescale image to maximum monitor size but it would involve a lot of platform specific code and I don't think it really worth it. |
|
I have a question regarding the changes made in the previous commit. Previously, we had the capability to load high-resolution images, but this process was time-consuming and resulted in significant lag in Neovide. With the new implementation, my understanding is that the image loading time has been reduced. However, I'm curious about the impact on Neovide's performance. Will it still be influenced by the size of the image, albeit to a lesser extent than before? To put it more succinctly, if we load a high-resolution image as a background and do not resize the Neovide window, will the overall performance, such as cursor movement and scrolling, remain the same as if no background image was loaded at all? Or rather, would loading a 10K image as a background result in the same performance as loading a 2K image? |
Yes, but only when resizing windows
Yes, it would be similar
Yes, performace would be very simular unless resizing So basicaly previous bottlenect was in image rescaling. Before I would scale a 10k image to much smaller resolution of a window each frame which take significan amount of time. However with the latest changes I cache the resized image and until the window size doesn't change it will just render cached image without any significan performace impact. |
| @@ -172,6 +176,35 @@ impl WinitWindowWrapper { | |||
| wrapper | |||
| } | |||
|
|
|||
| fn load_image(image: &str) -> Option<Image> { | |||
There was a problem hiding this comment.
It would be good to add some logging here with the filename it tries to load
| if image.is_empty() { | ||
| None | ||
| } else { | ||
| let skia_data = Data::from_filename(std::path::Path::new(image))?; |
There was a problem hiding this comment.
And after this line that it succeeded to load the file
| ); | ||
| if !success { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
And the final success and failure status
| if image.is_empty() { | ||
| None | ||
| } else { | ||
| let skia_data = Data::from_filename(std::path::Path::new(image))?; |
There was a problem hiding this comment.
I think we might want to use https://docs.rs/shellexpand/latest/shellexpand/fn.tilde_with_context.html here so that you can easily refer to the home directory
| // background, | ||
| // &pixel_region, | ||
| // screen_rect, | ||
| // ); |
There was a problem hiding this comment.
There's an extra comment here.
This relates to #342
vim.g.neovide_background_image = "E:\\downloads\\102075572_p0.jpg"It will have the same blur and opacity settings as single color background and look like this:
What kind of change does this PR introduce?
Did this PR introduce a breaking change?
A breaking change includes anything that breaks backwards compatibility either at compile or run time.