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

Improve and optimize the code for background images #551

Merged

Conversation

Vulcalien
Copy link
Member

Plus, fixed code duplication and organized code.

The problem: even if the background_type is not image, terminator will still print an error saying that the file was not found (when the file was deleted). To fix this, you should go to the config file and manually delete the background_image entry.

Solution: check if background_type == image before attempting to read the image file.

I also removed the duplicated code, that was identical to set_background_image.

In background_draw (which by the way seems to have bad performance) becomes unnecessary to check for background_type.

@mattrose
Copy link
Member

mattrose commented Dec 8, 2021

Thank you so much for fixing this up. I checked this background image code in in a bad state (in my defense, I wrote this while I was learning about terminator, and GTK and it was a long, frustrating time). I can certainly merge this code as is, as it's much better than the code I wrote, but...

I did notice that to get the picture to display I had to restart terminator. Is there any chance you could have the change apply as soon as the config changes? It should be just a matter of altering Terminator().reconfigure to pick up the changes. If not, lemme know and I'll merge the change as-is.

@mattrose
Copy link
Member

mattrose commented Dec 8, 2021

Also, any ideas you have for fixing up background_draw are welcome. I played around a lot with this function, but this was the only way I could get it to load the image reliably on every draw.

@Vulcalien
Copy link
Member Author

Vulcalien commented Dec 8, 2021

I've noticed the need to restart. I guess there is some way to update it, just like all the other settings update.
The performance issue I think is more difficult, because I don't really see any other way of having the background other than actively drawing it, which is the slowest part.
I'll work on the "update without restarting" first.

Update: first thing done, next I might try to improve performance. There was a setting, background_alpha that was no longer in use. Watching the old commits, it was added as a transparency value just for the bg image, but then background_darkness replaced it. That makes sense, since the background is either transparent or with an image, so one setting for both should be enough.

@Vulcalien
Copy link
Member Author

Vulcalien commented Dec 9, 2021

Found a way to optimize it! And I also had the possibility to use cProfile for the first time.

First, using Surface instead of PixBuf we get a free performance boost.
Now, the most CPU consuming operation is the scaling of the image. By default, the 'Filter' is GOOD, but as we saw that's way too slow. I've tried changing the filter to NEAREST or FAST and both are perfectly fine.
We should choose one of the filters (those two or others that I haven't tried) as the default for Terminator.

There is still a performance hit when loading the image (a terminal is initialized/reconfigured), especially noticeable for big files. I might work on that and see if it's possible to optimize that too.

Relevant:

@Vulcalien Vulcalien changed the title Only set background_image if background_type is 'image' Improve and optimize the code for background images Dec 9, 2021
@mattrose
Copy link
Member

mattrose commented Dec 9, 2021

Can you put the filter FAST patch in this PR as well? It looks really good. If people complain about image quality we can make the filter configurable later, but I'm pretty sure it'll be fine.

@mattrose
Copy link
Member

I love it! Thank you so much!

@mattrose mattrose merged commit d3125c2 into gnome-terminator:master Dec 10, 2021
@Vulcalien Vulcalien deleted the set-background-image-error branch December 10, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants