Skip to content
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

Add texture filtering options #1894

Merged
merged 6 commits into from
Nov 11, 2023
Merged

Conversation

Remmirad
Copy link

@Remmirad Remmirad commented Jun 3, 2023

Adds the possibility to add Texture filtering options to an image handle (should work with tiny_skia and wgpu) (Original Issue: #557 )

This is necessary when rendering smaller Pixelart or when trying to implement pixelbased image manipulation (As I am currently trying).
I asked on Discord and since nobody seemed to work at it I gave it a try.

I know it adds a little complexity to the rendering of images and imposes a rendering order of images with different filter options (Even though only inside a layer?) so if you think the feature is not interesting enough to justify these drawbacks its fine.
Just wanted to provide it. If its not completely unfitting I'd be happy to take feedback, since I am not very experienced with the graphics part.

Fixes #557.

@Remmirad Remmirad changed the title Implement texture filtering options Add texture filtering options Jun 4, 2023
@hecrj hecrj added this to the 0.11.0 milestone Jul 12, 2023
@hecrj hecrj added feature New feature or request rendering image labels Jul 12, 2023
@Remmirad Remmirad force-pushed the texture_filtering branch from a12a656 to c6d514b Compare August 4, 2023 10:45
@eswartz
Copy link

eswartz commented Sep 20, 2023

I discovered this after inadvertently implementing a similar thing (though as global options for the renderer, not per-image::Handle -- this seems like a better approach).

I think there may be a bug in this PR, though. In my app I animate images by creating a new handle every tick like this (and AFAIK this how it must be done, let me know if there's a better way):

struct IcedClientState { 
....
  screen_buffer: image::Handle,
...
}

and initialize like:

        let app = IcedClientApp { ...
            state: IcedClientAppState { ...
                screen_buffer: image::Handle::from_pixels(w as u32, h as u32, pixels).set_filter(TextureFilter{
                    min: image::FilterMethod::Nearest,
                    mag: image::FilterMethod::Nearest,
                }), ....
            },
        };

and update the image on a tick like:

   fn update(&mut self, message: Self::Message) -> Command<Self::Message> {
...
                    self.state.screen_buffer = image::Handle::from_pixels(size.0 as u32, size.1 as u32, new_pixels)
                        .set_filter(TextureFilter{
                            min: image::FilterMethod::Linear,
                            mag: image::FilterMethod::Linear,
                        });
...

and draw the view and image like so:

impl Application ... {
    fn view(&self) -> Element<Self::Message> {
...
                image(self.state.screen_buffer.clone())
                    .width(Length::Fill)
                    .height(Length::Fill),

This has two issues on this branch currently when using the wgpu renderer:

  1. There is a memory leak (RSS grows over time).
  2. The texture filter options are not changed by update although the texture content does change. (The code above is intentionally constructed to demonstrate the issue; of course I use the same settings in both cases in real life.)

I suspect the new HashMap is getting confused because (say) the GPU-driver-provided handle for the new image is reused and confusing the identity used by the map...? Though it doesn't explain why the image content changes.

Neither issue occurs in the tiny-skia renderer. Memory is steady and filter options from update() are visible as expected.

@Remmirad
Copy link
Author

Thanks for checking this. The cause for the image not updating is probably because I did not include the filter settings in the hash of the image thus the change detection misses that. I'll fix that.

For the memory leak I need to look into it. I might need to update some things here anyway to include new changes from main.

@Remmirad
Copy link
Author

Ok after looking into it I found that there are some problems in the wgpu part that I need to fix. (Should have tested this better, again thanks for doing so). The issue is not with the change detection but with the layers.

  1. I use a HashMap with random state so the iteration order changes from time to time
  2. The layers are cached so once a sampler is set in a layer it is never reset
  3. I simply add new layers assuming they will be just drawn which is not the case. (See interaction between backend.rs and image.rs)

I will attempt to do this properly and then we'll see.

@Remmirad
Copy link
Author

I tried to fix the mentioned issues. The size of a layer growths to four times its size (roughly on my machine: 350B ->1400B). This is unfortunate but the only alternative I can think of is to use an SamplerArray and handle the differentiation in the shader but it seems those are not available on the web.

@eswartz
Copy link

eswartz commented Sep 26, 2023

This works fine for me in practice, thanks!

@hecrj hecrj added the addition label Nov 11, 2023
Remmirad and others added 5 commits November 11, 2023 07:21
- Support only `Linear` or `Nearest`
- Simplify `Layer` groups
- Move `FilterMethod` to `Image` and `image::Viewer`
@hecrj hecrj force-pushed the texture_filtering branch from 710fd48 to a5125d6 Compare November 11, 2023 06:23
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks 🥳

I have refactored and simplified the code a bit in a5125d6. Specifically:

  • I have removed support for different minification and magnification filter methods. Normally, the strategy used for both is the same.
  • I simplified the layer groups. Instead of creating a new Layer per sampler, we can just add an additional sampler and its instances to the Layer. The uniforms can be reused.
  • I moved the FilterMethod from the Handle to the Image and image::Viewer widgets. This way, the method can be changed easily without recreating the Handle.

I have also added a simple checkbox to the "Image" step in the tour example to enable nearest interpolation.

A nice side-effect of these changes is that now we can use nearest interpolation for SVG, which should fix blurriness when using the Svg widget in some cases! 🎉

Let me know what you think and I believe we can merge this.

@Remmirad
Copy link
Author

Thanks for the refactoring/overhaul. Looks and works great. Good that you could clean up the layer stuff a bit. I was not very happy with the previous state but lacked an idea to make it better.
Makes sense to move the Filter out of the handle. Admittedly putting a rendering-detail together with the image data was a bit weird.
Can be merged I think.

@eswartz
Copy link

eswartz commented Nov 11, 2023

* I have removed support for different minification and magnification filter methods. Normally, the strategy used for both is the same.

Eep, this was exactly the use case I came to this PR for, though. I'm working on a project (related to pixel art) where I want minification to be "smooth" and magnification to be chunky. Is it possible to restore this?

@Remmirad
Copy link
Author

It would be possible of course. Its just not that nice with the 4 samplers. Is linear min-sampling even desirable for pixel-art? When I hardcode min-filter to linear it again started to look all blurry when scaled down. (Which I though one tries to avoid here)

@eswartz
Copy link

eswartz commented Nov 11, 2023

Well, I'm speaking mainly of use cases like shrinking down to the range of e.g. 1/2 or 1/4 the size -- I want to see "all" the pixels accounted for. A nearest filter (as used for magnification) would cause most of the pixels to disappear.

I suppose, without making such a change, I could change the (single) filter dynamically based on the apparent scaling.

@hecrj
Copy link
Member

hecrj commented Nov 11, 2023

I suppose, without making such a change, I could change the (single) filter dynamically based on the apparent scaling.

That sounds like a valid strategy, and you will have more control that way.

Let's merge then!

@hecrj hecrj merged commit 178521e into iced-rs:master Nov 11, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable bilinear filtering for Image
3 participants