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

Multiple svg images render blurry #3501

Open
thequver opened this issue Oct 24, 2023 · 11 comments
Open

Multiple svg images render blurry #3501

thequver opened this issue Oct 24, 2023 · 11 comments
Labels
bug Something is broken

Comments

@thequver
Copy link

Describe the bug
When adding multiple svg images from the same source but with different size, the latter image renders blurry as if it takes the texture from the first added (smaller) image.

To Reproduce
Code to reproduce:

                const IMG_SRC: egui::ImageSource = egui::include_image!("./img.svg");
                ui.add_sized(egui::Vec2{x: 20.0, y: 20.0}, egui::Image::new(IMG_SRC));
                ui.add_sized(egui::Vec2{x: 200.0, y: 200.0}, egui::Image::new(IMG_SRC));

Image i used to test:
img

Expected behavior
Both vector images must look sharp and render at respective for its size resolution

Screenshots
Screenshot

Desktop (please complete the following information):

  • OS: Win11

Additional context
Reproduced on win11, android 10, eframe, egui-winit + egui-wgpu implementation, egui 0.23.0, master branch (as the time of writing)

@thequver thequver added the bug Something is broken label Oct 24, 2023
@chriscate
Copy link
Contributor

This appears to be due to the the caching done by the ImageLoader. If you call ctx.forget_image(IMG_SRC.uri().unwrap()); between your two ui.add_sized() calls it will work as expected based on my testing. There is also a TODO here for automatic cache eviction.

If anyone has guidance on how to implement that automatic eviction I'd be happy to take a shot at it.

emilk pushed a commit that referenced this issue Dec 20, 2023
Update `resvg` from v0.28 to v0.37. 
Remove related, unnecessary entries from `deny.toml`.

⚠ In example `images` ferris is scaled differently, but I guess that now
it scales in expected way (takes all available space; before this PR it
takes up to space that, was available at first render- it does not
upscale).

This PR is minimal adaptation to new `resvg` api and small related
simplification, however it should be considered to update loaders
(currently if svg image initially was small and was scaled up it will be
blurred, see #3501). As svg image
now scales over render size, problem will be more often seen now.

(currently `SvgLoader` theoretically should rerender for different sizes
(but I guess it will result in memory leak in that case), but refreshing
is stopped earlier in `DefaultTextureLoader`).

I have initial version of loaders update, that will fix issue with svg
scaling (and also enable e.g. reloading image if file has been changed),
I will submit these changes in separate PR once this one is merged.

Closes <#3652>.
@YgorSouza
Copy link
Contributor

I was looking into this, and found that the SVG loader actually includes the size hint as part of its key:

#[derive(Default)]
pub struct SvgLoader {
cache: Mutex<HashMap<(String, SizeHint), Entry>>,
}

Which would solve this problem, but also sounds kind of dangerous, because if you have an SVG that fills the available space, you would end up with hundreds of copies at different sizes as the UI is resized.

But in any case, it isn't working as intended, because the image loader is called by the texture loader, which does not take the size into account.

fn load(
&self,
ctx: &Context,
uri: &str,
texture_options: TextureOptions,
size_hint: SizeHint,
) -> TextureLoadResult {
let mut cache = self.cache.lock();
if let Some(handle) = cache.get(&(uri.into(), texture_options)) {
let texture = SizedTexture::from_handle(handle);
Ok(TexturePoll::Ready { texture })
} else {

So one way to work around this would be to copy this default texture loader to your code and change it to take the size into account like the SVG loader does, then add it to the context using add_texture_loader. Maybe this should be added to egui itself, but it seems like it would be hard to find a balance between CPU and RAM usage and image quality that would work for every use case.

@molenick
Copy link

molenick commented May 16, 2024

I'm experiencing this as well with the SvgLoader and am trying to find a good interim solution. @YgorSouza's post has me on the right track, instead of using a TextureLoader I'm applying the same line of thinking to create a custom ImageLoader copied from egui_extras SvgLoader.

So far I've noticed the following behaviors from debugging the caching of my custom svg loader:

  • Loading an svg uri 2x, first with small size and second with large size results in blurry image
  • Loading an svg uri 2x, first with large size and second with small size results in crisp image
  • Loading an svg uri multiple times with many sizes, I never see additional cache entries added

I don't think anything special is happening here on my end, I'm just using the painter api to paint into a rectangle. I think the next thing to wrap my head around:

  • how does first-time painting of an svg into a rectangle influence the SizeHint of a cache entry?
  • why do I never see multiple cache entries for my svg uri, when I have requested paints at multiple sizes? - I think this is because we only cache when reading from disk, which happens once.
  • is there a way to load an svg from disk with an explicit size hint (instead of the implicit size hint from first read)
  • in general, in what scenarios will a loaded image have multiple cache entries?

@molenick
Copy link

Ok, I see what I was missing: TextureLoader is the abstraction level that paint uses to cache an svg.

@chianti-ga
Copy link

any news about this issue ?

@molenick
Copy link

molenick commented Aug 7, 2024

Hi @chianti-ga I was able to work around this issue by implementing my own loaders where I changed the cache-key to represent both file uri and rendered size, so that once I rendered a svg at a given size it remains cached unless otherwise forgotten.

@chianti-ga
Copy link

Hi @chianti-ga I was able to work around this issue by implementing my own loaders where I changed the cache-key to represent both file uri and rendered size, so that once I rendered a svg at a given size it remains cached unless otherwise forgotten.

@molenick why not submit a pull request with this solution? I think it's a straightforward way to address the issue.

@molenick
Copy link

@molenick I see my solution as being custom to the need of my project, and after working with it it seems to me like the Loader API is the intended api to customize caching behavior to suit your app's need.

However it seems like this is a common use case given the popularity of this issue, so if the maintainers think this is a worthwhile addition I'd be happy to contribute.

I think the general behavior of this loader is "cache by uri and render size" instead of "cache by uri" and is pretty simple to write. I think there are a lot of people who "just want to use egui" and are surprised by the default caching behavior so this could be a good change for users that want to render svg at different size without worrying about egui's caching subsystem.

@molenick
Copy link

@chianti-ga Whoops, realized I responded to myself up in that last comment

@barries
Copy link

barries commented Oct 14, 2024

Yah, egui's current behavior it's clearly buggy--images should not render blurry or not based on what size you happen to "load" them at first. Image loading should be size agnostic, then image rendering should take size into account, and the resulting textures should be cached internally with size as part of the key.

Having to work around this with a lot of custom code is probably not specific to your app, @molenick, although perhaps the specific way you do it is.

@HactarCE
Copy link
Contributor

HactarCE commented Dec 4, 2024

I recommend clearing the cache automatically when adjusting the UI scale so that images are not cached for every possible UI scale.

EDIT: #3453 may be relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

No branches or pull requests

7 participants