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

Static blur and rounding effect for dash-to-dock #533

Merged
merged 12 commits into from
Mar 5, 2024

Conversation

a-parhom
Copy link
Contributor

I've added static blur from Blyr and a rounding efffect, based on Rounded WIndow Corners and LightlyShaders.
I've also added to dash-to-dock settings a switch to enable/disable static blur and a scale widget to set corner radius.
The result of using static blur with rounding effect is on the screenshot:
Знімок екрана 2024-02-24 о 01 50 43

@a-parhom
Copy link
Contributor Author

I think the same kind of static blur could be used for applications with rounded corners.

@aunetx
Copy link
Owner

aunetx commented Feb 24, 2024

Wow! Thank you very much! That is the single biggest external contribution I've ever gotten on this extension :) and the code seems very clean!

I will test it thoroughly before merging this, and I think after merging I may try to allow the user to switch between "native blur effect" (which may be more efficient? but that's only a guess), "non-native blur effect" (this one, and then make the user choose the radius) and "dynamic blur effect" (native) when available; for each element! So this could be used for dash-to-dock, the panel (useful when users use a theme that has rounded panel), applications, and even (with some more work...) for popups and notifications!

I have two little questions:

  • I see you implemented rounded corners and blur effect as two separate clutter effects; do you think it could be more computationally efficient to merge these two into a single effect that has the two functionalities? I don't really know if it changes anything to be honest so just asking
  • doesn't it experience memory leaks? I merged Add an effects manager for custom clutter effects to prevent ram bleedings #480 some months ago because the color and noise effects (both Clutter.ShaderEffect) did not get properly released by mutter when destroying the effects, creating immense ram bleedings... I suspect this should happen here too, so I will see how to integrate this PR with the effects manager I created before!

Again, thank you VERY much for your contribution, it is really a huge milestone for this extension!!!

@aunetx
Copy link
Owner

aunetx commented Feb 24, 2024

Some problems I found:

  1. the new blur effect is not similar to the native blur, due to the fact that the native blur blurs the entire background image and then only extracts a part of it for the dash to be shown (I mean, not for the dash as static blur was not implemented before, but for the panel for example).
    This means that for an equal amount of sigma and brightness, the dash will not blend into the overview blur -- this could be not very important for some users, but actually makes the effect a lot less visually appealing (this can be seen with the sigma value not changing the blur that much; for example on my machine, to have a similar effect on the dash and on the overview I need: sigma=7 for the overview, sigma=100 for the dash.
    Moreover, this would also mean that using this for application blur would make the blur "choppy" when moving the window around, meaning that it would discover only its immediate background (and not its surroundings).
    However, there should be a quite simple and more or less efficient way to correct this: if the sigma is 30px, we then need to extend the image by 30px in all directions (so for a dash that extends with x_min=400, x_max=600, y_min=800, y_max=880 and with a 1000×900px background image, and 30px sigma, we need to blur the part of the background from x_min=370, x_max=630, y_min=770, y_max=900 and then remove (30,30,30,20) pixels around the blurred image to have the resulting correct blur effect. This is quite a shitty way to do it, but I don't have any better idea to implement it...

  2. the rounding effect does not seem correct on my machine (the corners are not smooth, as shown in the following image). I have something that kinda worked in A corner effect, working... when no blur is used. #265, so I will try to compare the two implementations to take the better of them!
    image

  3. there seems to be problems with dash-to-dock background coming back again sometimes, it may be due to a change in this PR because it does not happen with static blur disabled, so I will try to find the culprit (it seems like this.dash._background.style becomes null at some point when override-background is disabled, which should not happen). At least when override-background is enabled there is no problem, which is the most important.

  4. when enabling the Dash to Dock blur, it first creates a completely wrong effect (video capture), and then when disabling and enabling again the static blur it comes back to a normal effect. I guess this is due to a race condition in the initialization of the effect?

  5. when setting sigma=0, the blur effect is activated and quite interestingly stronger than with sigma=1! I will simply disable the effect when sigma=0 as it is probably what the user expects

  6. the effect sometimes becomes entirely black... I don't know why this happens, and cannot reproduce it consistently -- and this does not seem to be a styling issue

  7. when using the dash in vertical mode (for example putting it on the left of the screen), the blurred image seems to be ~20px higher that it should be -- but that only happens with the Hide Top Bar extension enabled, so that's really a baby issue

Some other points I will probably change:

  1. the effects currently need to be created in three times: first the horizontal blur, then the vertical blur, and then the corner effect. While this is technically simpler, this also means that using this in more places will be hard (for example, managing applications blur with a single effect already feel like psychological torture, so adding two more could make it feel like a living hell). I may try to create a meta-effect that manages the three effects all at once, maybe that will be better?

Quite interestingly though, there does not seem to be any memory leak when destroying and re-creating the effects, so the second point in my previous comment should be OK! That's already a very big pain that is removed to be honest :)

And the first point in my previous comment is not very important finally, because... that's already perfect if it works like that, there is no need to over-engineer it (as I usually do) :D

@aunetx
Copy link
Owner

aunetx commented Feb 24, 2024

I think I fixed the point 2.; but that's really a not very clean fix (and I don't know if it is reproducible): I simply shift the position of the center by 2 positive pixels (if it is an addition) or by 1 negative pixel (if it is a subtraction). I have no idea why this works, but this works so at least there is no more visual glitch.

@aunetx
Copy link
Owner

aunetx commented Feb 24, 2024

I fixed the point 5. too

@aunetx
Copy link
Owner

aunetx commented Feb 24, 2024

I also fixed various crashes, so with some testing I will see because this may fix 6. too

(update: not fixed, this seems to happen sometimes when new windows are created)

@a-parhom
Copy link
Contributor Author

a-parhom commented Feb 24, 2024

I've implemented the effects separately, because the blur effect needs two passes: vertical and horizontal. I've tried to merge them in one glsl shader, but failed. It seems, that the effect itself needs to be rewritten to work in one pass.

Merging corner effect is not hard at all, though. It could be merged and applied after second pass, for example.

I also don't know if there is significant difference in calling one effect with merged features instead of three calls. In KDE there is a special effect, that shows the GPU and CPU load when rendering frames. I'm not sure if something similar exists for GNOME, so the performance could be measured in some way.

@a-parhom
Copy link
Contributor Author

It looks like the point 4, that you have mentioned, occurs because the dimensions for the effects are not set correctly for some reason. Seems like we need to connect to some event and call update_size() method.

@a-parhom
Copy link
Contributor Author

Point 1: I've tried to do what you suggested, but it seems the problem is that Blyr shader works with maximum sigma value 30 (that is why I convert the value in blur_effect.js in sigma setter). And since it uses hardcoded weights, it seems to be hard to reimplement it (though I'm not an expert here).

But I can try to implement the blur effect from Clutter. I'm not sure if it is possible in GJS, though, since it uses Clutter C API for some things. But I'll try to tinker with it.

@a-parhom
Copy link
Contributor Author

a-parhom commented Feb 25, 2024

The shader itself from Clutter's blur effect works. It also needs two passes, but seems to work correctly with any sigmas. Also, Clutter downscales textures and sigmas to increase efficiency of the effect.

I'll see if I can implement the downscaling as well, and also will try to make it act and look same as the native effect (point 1 from your second comment).

@aunetx
Copy link
Owner

aunetx commented Feb 26, 2024

Well, thank you very very much for your work!

About what you said in your last point: downscaling would be pretty great, but I have no idea how to do it -- I tried multiple times to convert the Meta.BackgroundActor object to some kind of image object, but I have not found a way so far. So if you have an idea, that I would be very excited to hear about it as it may help me in other parts of the code!

An I will push the changes I did to your code today (mainly crash fixes and my formatter doing its job too), I hope this does not introduce conflicts with your repo!

@aunetx
Copy link
Owner

aunetx commented Feb 26, 2024

Point 3. was fixed, it was visibly caused by the crashes in the extension (actually no)

@ryzendew
Copy link

ryzendew commented Feb 27, 2024

image
seems off center for me going more to the top

with static blur off
image

after logging in this happens

image

@a-parhom
Copy link
Contributor Author

a-parhom commented Feb 27, 2024

I haven't found a way to resize the texture like it is done in Clutter's blur effect: it creates another pipeline with smaller offscreen framebuffer and renders the texture there. I don't see any way to do this in GJS, at least for now.

I've managed to get the image from actor's texture, but I don't think it can be useful in any way, because getting the image from GPU, resizing it by CPU and pushing it back is very inefficient process itself. In fact, it can introduce more lag, then the profit we will gain from the texture scaling approach.

But I'll try to do what you mentioned in your point 1 with the shader from Clutter's blur effect and see if it can improve the looks and consistency of the blur.

@@ -269,6 +269,11 @@
<default>false</default>
<summary>Boolean, whether to disable blur from this component when opening the overview or not</summary>
</key>
<!-- CORNER RADIUS -->
<key type="i" name="radius">
Copy link

@alissonlauffer alissonlauffer Feb 28, 2024

Choose a reason for hiding this comment

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

FWIW, Gnome 46 will replace sigma with radius in blur APIs. I'm not sure if we are going to reserve the radius key for Gnome 46+, or just keep the sigma key untouched, and assume radius = sigma * 2. See #532 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think, we could use radius for blur settings and corner_radius for the rounding effect in GNOME 46.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes! I think we should use corner_radius and replace sigma by radius starting from GNOME 45, so that no change is required for it afterwards. Once this PR is merged, I will add the changes for GNOME 46 for the other blur effect!

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 2, 2024

I've replaced the shader, but failed to implement your recommendations from point 1. Maybe I'm doing it wrong. The code is all there, it's just disabled (the returned offsets are all 0), so you could try make it work properly.

Also, blur_effect.js contains commented out part related to my experiments with getting texture from actor, in case it can be useful. Otherwise, fell free to remove it.

The blur itself, even without implementing point 1, looks very similar to native blur, though. So I consider this as an improvement anyway.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 2, 2024

Sorry, I didn't run it through any code formatter, so the last commit may introduce new formatting issues.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 2, 2024

I've tried to fix issues with the Hide Top Bar extension and issues with blur position from RyzenDew's comment. Seems to work fine now, at least for me.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

I've merged the effects to one, kind of. The effect still needs two passes, so it adds the second instance of itself to the actor through overridden vfunc_set_actor method. The rounding is done in the second pass.

I've also managed to fix the enable/disable bug from point 4, as well as some more positioning bugs with custom shell themes.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

Looks like the only remaining big annoying issue is the styling one. I'll try to look into it this week.
@ryzendew please, test the latest changes.

@aunetx
Copy link
Owner

aunetx commented Mar 4, 2024

Thank you very much for your work! I will test it right now, and will push soon after prettifying this!

For the styling bug, do you talk about the point 6.? I tried quite a lot to find the cause, to no avail for the moment... It seems like when it happens, the blur_effect.direction is null, which is kinda weird (and may be the cause, but changing them to 0 and 1 does not resolve the bug). I will try and see if it changed with your recent changes!

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

I'm talking about the point number 3. I'm getting darker background until I switch blur type back and forth. And it does not look like the dark style from the Override background switch in settings.

Maybe, it is connected with the point 6 somehow.

@aunetx
Copy link
Owner

aunetx commented Mar 4, 2024

It works really well, thank you very much! I'm not getting bug 3 right now although I had it consistently before, so maybe something changed? I am not getting bug 6 either by the way, so nearly everything is fixed!

I think it may be good to do some renaming:

  • the radius for the blur effect should probably be used instead of sigma, as Shell.BlurEffect is using this new naming scheme for GNOME 46; but then we need to use radius = 2*sigma
  • the corner radius should probably be renamed corner_radius accordingly

I will do this, along with adding a scaling factor for the blur radius (so it becomes monitor-independent)!

@aunetx
Copy link
Owner

aunetx commented Mar 4, 2024

I think this is nearly finished! The bug 3. is kinda annoying, but it does not happen on my machine at all any more, and I don't think it is a huge blocker if it ever happens as there is an option (activated by default) which prevents the bug from happening at all anyway.

What is your take on this?

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

It looks like sigma is not updated in the static blur mode now.

Edit: Also, looks like sigma value behaves differently for native blur and the static one now.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

Regarding point 3: I'm still having it, but maybe it is because I test in a virtual machine, or because my Ubuntu is on a development branch and is halfway updated to GNOME 46. If you can't reproduce it, then maybe it's just some weird corner case because of my configuration.

@aunetx
Copy link
Owner

aunetx commented Mar 4, 2024

Yes I'm sorry, I badly changed the behaviour. This should be corrected now!

I get it for the issue 3, then I guess the best would be to merge this, and see if there is any issue that is opened on this topic.

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 4, 2024

Looks good to me now.

@aunetx
Copy link
Owner

aunetx commented Mar 5, 2024

Me too! I merge this :) thank you so much for your contribution!!

I will later try to offer the option to use this new blur effect for the panel, which would permit the use to use rounded corners for the panel too (so that floating panels would not be ugly!!)

The main problem is that there is now possibility to have blur everywhere, so I have no valid excuse not to implement blur for notifications, popups, ... :D

@aunetx aunetx merged commit d72db05 into aunetx:master Mar 5, 2024
@aunetx
Copy link
Owner

aunetx commented Mar 5, 2024

@a-parhom I have a little question: I've been trying to use this blur effect for the panel, but there seems to be a problem when I set the clipping of the image to be very close to its border. It looks like the pixels are counted as white (ie not counted) at the border; however this means using it for little blurred objects (eg the panel, which is less than a hundred pixels high) is not very adequate. Is it right? If so, I will try to tweak the effect to simply not count those pixels!

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 5, 2024

Could you show me the code, please, or maybe, just a screenshot of the result, because I can not quite understand the problem.

@aunetx
Copy link
Owner

aunetx commented Mar 5, 2024

Of course! I began some dirty work on #539, and you should see that the panel blur is very "white" after using this code.

De-commenting what I did in the GLSL shader (and commenting the 3 lines after it instead) should have fixed the bug if I understand the shader correctly, but it is not near good enough (for some reason the effect is somewhat transparent).

I will try to fix this, but that's really not very important as it works for the dash which is the most important! It was just to ask you if I did something wrong using the effect :)

@a-parhom
Copy link
Contributor Author

a-parhom commented Mar 6, 2024

I've tried to do it myself, and here is what I've found out:

When I output with console.log width and height returned by panel_box.get_size() (that are passed to set_clip, as well as to effect settings and background dimensions), I'm getting values 2992 and 64.

But when I output with console.log width and height of the actual texture, that is passed to the effect in vfunc_paint_target, I'm getting values 2995 and 67.

So, it adds some empty pixels to the texture. Maybe it's a bug in Clutter, I don't know.

@aunetx
Copy link
Owner

aunetx commented Mar 6, 2024

Okay, thanks for the investigation! I will see what I can do about it, I have an idea to solve this problem (no time to implement it right now but this will come)!!

@a-parhom
Copy link
Contributor Author

Of course! I began some dirty work on #539, and you should see that the panel blur is very "white" after using this code.

De-commenting what I did in the GLSL shader (and commenting the 3 lines after it instead) should have fixed the bug if I understand the shader correctly, but it is not near good enough (for some reason the effect is somewhat transparent).

I will try to fix this, but that's really not very important as it works for the dash which is the most important! It was just to ask you if I did something wrong using the effect :)

I've implemented a workaround to the problem with additional empty pixels around the effect's texture in #540.

In general it behaves good, but seems to suffer from the similar bug with transparency when the session starts or the blur type is switched. But as soon as any blur parameters are changed, the transparency gets to normal.

This is weird, because the dash-to-dock blur does not suffer from that bug, despite using the same static effect with the fix for additional empty pixels in texture.

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

4 participants