search

lucasmerlin / egui_skia

Render egui with skia!
MIT License
55 stars 13 forks source link

CPU rendering is a little off #1

Closed daniel5151 closed 2 years ago

daniel5151 commented 2 years ago

Hey there!

I know you're already aware of this issue (it's even mentioned in the README), but I thought I'd open a tracking issue regardless so I could express my interest in seeing this fixed.

I just managed to integrate egui_skia in a project of mine (https://github.com/daniel5151/wide-libretro), and I'm seeing the same rendering issues as in examples/cpu.rs.

Thanks again for implementing this! Even in its current somewhat broken state, it should be sufficient to start doing what I wanted to do :)

Capture

lucasmerlin commented 2 years ago

Hi, thanks for opening this issue! I think this might actually be a problem with skia itself, since it works perfectly fine with a gpu backend.

I'll try to make a minimal reproduction of this and open an issue with skia once I get back from holidays.

lucasmerlin commented 2 years ago

I think I nailed down where the issue is caused: So egui is using the following font texture: Managed(0) In the top left corner is a tiny 1px white dot. When drawing a shape without texture (e.g. a window background), egui uses the texture coordinates 0.0,0.0 which should select the color from the white dot. For some reason that's not working correctly on the cpu (maybe a floating point error or something like that?).

I was able to create a skia fiddle that shows this issue: https://fiddle.skia.org/c/114a0ba43dadecad43a6630105d7b5c8

And I created a skia issue: https://bugs.chromium.org/p/skia/issues/detail?id=13706

lucasmerlin commented 2 years ago

Hi! I made a workaround and published version 0.1.1. This adds a cpu_fix feature that adds a workaround for the skia problem. Can you confirm this also fixes it for you @daniel5151?

daniel5151 commented 2 years ago

Wow, would you look at that! Awesome!

Capture

Admittedly, it does seem like there is quite a perf hit to enabling the feature (especially after opening a bunch of panels). That's totally fine for use-cases though.

Thanks again!

lucasmerlin commented 2 years ago

Nice! I tried to improve the performance of the splitting function I wrote for the fix, but I'm not sure if it made a huge difference. I think the main problem is that skia has to render a lot more single Meshes now, not sure if there is a workaround that does not need so many new Meshes. You can try the improvements on the improve-raster-performance branch, let me know if this improves it for you!

daniel5151 commented 2 years ago

Hmm, i'll be honest, I don't notice too much of an improvement with that new code...

I wonder if you can eek out more perf by reducing the number of allocations you're doing? Maybe by recycling allocated Vecs between frames when possible?

lucasmerlin commented 2 years ago

I implemented a different workaround now, and it turns out, it's not the workaround that slows things down. Rather the bug itself probably causes skia to skip some expensive rendering work. In the improve-raster-performance branch, you can try changing line 291 to 293 in painter.rs from 0.5 to 0.0 and the bug should reappear and things should speed up significantly, while nothing else changed.

So unfortunately I don't think there is an easy way to speed this up on the cpu, and also I don't expect it to speed up even once the underlying skia bug has been fixed.

I'll close this issue now since the graphical problems have been fixed.

daniel5151 commented 2 years ago

Makes sense. Thanks for digging into this!