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

Fallback to background color when background image not found #46

Closed
wants to merge 4 commits into from
Closed

Fallback to background color when background image not found #46

wants to merge 4 commits into from

Conversation

DerekSauer
Copy link

@DerekSauer DerekSauer commented Feb 21, 2024

Confirms if the background image was actually loaded and logs a warning if not.

If no asset exists for a background image, fall back to background color.

Tested by having a background image where hyprlock expects it. Background is rendered properly. Deleted the background image from disk and hyprlock reverts to background color. Deleted background color in config and hyprlock reverts to default color.

Fixes #45

Disclaimer: I haven't written any C++ code since I was in college 20 years ago so please review this.

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -6,21 +6,18 @@ CBackground::CBackground(const Vector2D& viewport_, const std::string& resourceI
}

bool CBackground::draw(const SRenderData& data) {
if (!asset)
Copy link
Member

Choose a reason for hiding this comment

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

actually, I'd check for the resourceID being empty here too. getAssetByID does a lookup and if the resourceID is empty why would we lookup

Copy link
Author

@DerekSauer DerekSauer Feb 21, 2024

Choose a reason for hiding this comment

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

The resourceID isn't empty.

In the resource gatherer if the image cannot be found on disk it skips assigning an ID and bails early just like if the background path in the config was empty since there isn't actually a resource to be found.

In CRenderer::getOrCreateWidgetsFor() it uses background: + PATH as the resource ID. PATH does contain a valid value (the path where we expected the resource to exist on disk) but no associate resource ever got loaded.

Checking for an empty ID will always return true since there is an ID, its just looking for a resource that was never there.

Copy link
Author

@DerekSauer DerekSauer Feb 21, 2024

Choose a reason for hiding this comment

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

Another approach would be to add an additional check in CRenderer::getOrCreateWidgetsFor() to see if the desired file exists and push an empty string ("") as the resource ID if it does not. That would allow checking for an empty resource ID in background render and skip the resource lookup, which is wasteful since the resource will never exist anyway.

I didn't do it that way since I wanted to error check the PNG load to guard not only against missing files but also corrupted files and OOM and such. Cairo has a bunch of error states it can return when loading an image.

Gonna try something.

Simply checks if the background image exists on disk and if not fallback
to rendering the background color.
@DerekSauer
Copy link
Author

Now it only checks if the image file exists. If it does not, it assigns an empty resource ID allowing immediate drop to background color fallback instead of attempting a pointless asset lookup.

This won't protect against corrupt images or JPEGs pretending to be PNG files but why would someone do that for their lockscreen, they'd only be hitting themselves.

Cairo still tries to load the file that isn't there but I guess it doesn't actually care if the load succeeded or not and OpenGL doesn't seem care if it gets nothing for data and creates an empty texture. You're still paying for some empty buffers but fixing that can wait for a refactor of the resource system.

I typed my login password so many times while working on this I can no longer type it properly.

@vaxerski
Copy link
Member

I typed my login password so many times while working on this I can no longer type it properly.

general:grace = 1000 and any mouse movement will unlock xd

This won't protect against corrupt images or JPEGs pretending to be PNG files but why would someone do that for their lockscreen, they'd only be hitting themselves.

What if, we generate the asset, but attach a flag failed or something to indicate it failed generating

If the background image defined in the config file at `background:path`
cannot be found, render the lock screen background with the color
defined at `background:color`. If color is not defined fallback to
default color.

Added `valid` flag to `SPreloadTarget` and `SPreloadedAsset`.

If Cairo is unable to load an image, for whatever reason, the
PreloadTarget is invalidated. The Resource Gatherer will not attempt to
create a GL texture for invalid Preload assets and the resulting texture
will be invalid, allowing later render functions handle a bad texture as
they see fit. In this case a bad background texture is replaced with the
solid fill color.

This should allow safe fallback to a color without a bunch of Cairo and
GL buffers hanging around for no reason.
@DerekSauer
Copy link
Author

Again give it a good look over if you could. I'm a Rust cultist and not having the borrow checker to slap me around when I'm playing dangerous with derefs makes me feel like I'm doing a Tom Cruise stunt with no harness.

@vaxerski
Copy link
Member

ye uhm oops I kinda uhh conflict hehe

@DerekSauer
Copy link
Author

lol, your last few commits invalidated my reason for doing this anyway and much more elegantly. blurry screenshots are a go

@DerekSauer
Copy link
Author

Recent changes has made this PR no longer necessary. Thank for all your work @vaxerski, I don't know where you find the energy.

@DerekSauer DerekSauer closed this Feb 27, 2024
@DerekSauer DerekSauer deleted the background_fallback branch February 27, 2024 15:44
@DerekSauer DerekSauer restored the background_fallback branch February 27, 2024 15:44
@vaxerski
Copy link
Member

me neither

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.

Missing background image leaves empty lockscreen
2 participants