[WIP] bumpmaps - rework and turn on #8428
Conversation
This reverts commit a79ca9b. Was getting segfaults that appear to be because of this.
There was a problem hiding this comment.
Hi @brownsr, I was seeing if I could test this with Cinnamenu, and it appears to only show the desktop wallpaper. I'm applying this to the popup-menu-content class (only one that seemed to show the bump map), and opacifying every actor underneath.
Speeds seem fine with no noticeable speed penalty unless a large blur count is put in for test purposes.
I'm also concerned about performance. My initial impression with no effects applied to CSS is performance is regressing and window rendering becomes further out of sync with the cursor. This is noticeable for me on my Xeon/Nvidia desktop I do most benchmarking on.
I am applying this on top of master + linuxmint/muffin#410 + linuxmint/muffin#397 + #8269 + #8300. At least the first issue might be a base branch cause, but at the same time could also be Cinnamenu's actor hierarchy, so not sure but can test more this weekend, along with some compositing benchmarks. When returning to Cinnamon without this PR applied, I can only describe it as "much snappier".
| // bumpmap_path, | ||
| // NULL); | ||
| { | ||
| if ((theme_node->background_blur > 0 || theme_node->background_bumpmap) |
There was a problem hiding this comment.
Might be good to do these logical comparisons when the change in CSS is detected, instead of on every paint.
There was a problem hiding this comment.
Perhaps, if I could work out how to do that, but there's only a couple of very fast and cheap tests here, so I can't imagine any practicable difference.
There was a problem hiding this comment.
i.e. the work to set up the effects is only done the first time through, and after that there there is just a check to see if it exists. So only the first time through takes time if either of the effect types are there in the CSS, and after that there is just a simple if test, which should take a miniscule time. I can't see any effect on performance myself if the effects are not in the CSS.
There was a problem hiding this comment.
I see an impact on performance immediately upon using Cinnamon with no CSS changes. You are adding a lot of code to St's paint function. Please try testing in higher resolutions.
| { | ||
| st_widget_recompute_style (widget, old_theme_node); | ||
|
|
||
| st_widget_add_background_effects(widget, old_theme_node); |
There was a problem hiding this comment.
Maybe we can check if the theme node is eligible before calling st_widget_add_background_effects? We should know after st_widget_recompute_style.
There was a problem hiding this comment.
I think that will just move the tests in the function to be done preceding it. I don't think there will be any improvement.
There was a problem hiding this comment.
After #8230 is merged, you could piggy-back off of the change condition I added, to give some idea. I find it hard to believe you can say there will be no improvement on something you haven't tried yet.
| static void st_widget_add_background_effects (StWidget *widget, | ||
| StThemeNode *old_theme_node ) | ||
| { | ||
| StThemeNode *new_theme_node = st_widget_get_theme_node (widget); |
There was a problem hiding this comment.
We could probably make st_widget_recompute_style return the new_theme_node passed as an argument to this function and avoid a function invocation in the paint cycle.
There was a problem hiding this comment.
Yes, but this function is also called without being preceded by recompute_style, so that won't work in that instance.
There was a problem hiding this comment.
Sounds like a lot of overhead for a feature maybe 25% of Cinnamon users will take advantage of (optimistically).
There was a problem hiding this comment.
Yes, but remember I am only re-implementing something that was already there and had to be turned off last time because it was broken. There's no hurry as far as I am concerned. With all the major performance / latency stuff that you've done, the last thing I want to do is to compromise that. So right now I am waiting for the dust to settle, maybe next release, before looking at this PR again and seeing how it might be improved.
| { | ||
| if (widget->priv->background_blur_effect != NULL) | ||
| { | ||
| g_object_run_dispose (G_OBJECT (widget->priv->background_blur_effect)); |
There was a problem hiding this comment.
In st_widget_recompute_style, we know if the style has changed, so it might be good to use that boolean state instead of assuming any processing is needed because there is an old theme node.
There was a problem hiding this comment.
Yes, I can see that might be possible in that case. But this function is also called when there is no style change. Could potentially separate out into two functions, perhaps i.e. have one dedicated to just the case where there is a style change. I can't see there would be a speed improvement, but it could possibly make the code a tiny bit easier to follow, probably at the cost of a bit more code.
There was a problem hiding this comment.
We have a private struct used by this class, so we can check if the style has changed, and guard this more. This is a paint function, and it will impact latency, even when the user is not interacting with the Cinnamon UI - it is part of the stage.
|
@jaszhix Thanks for the review. It may take a while for me to work through
it as I have a heavy workload the next two days and then I am away at the
weekend. your detailed comments are much appreciated :-)
…On Thu, 7 Mar 2019, 03:26 Jason Hicks, ***@***.***> wrote:
***@***.**** commented on this pull request.
Hi @brownsr <https://github.com/brownsr>, I was seeing if I could test
this with Cinnamenu, and it appears to only show the desktop wallpaper. I'm
applying this to the popup-menu-content class (only one that seemed to
show the bump map), and opacifying every actor underneath.
[image: image]
<https://user-images.githubusercontent.com/6859057/53929176-740cc280-4052-11e9-814d-2b731f6f6c96.png>
Speeds seem fine with no noticeable speed penalty unless a large blur
count is put in for test purposes.
I'm also concerned about performance. My initial impression with no
effects applied to CSS is performance is regressing and window rendering
becomes further out of sync with the cursor. This is noticeable for me on
my Xeon/Nvidia desktop I do most benchmarking on.
I am applying this on top of master + linuxmint/muffin#410
<linuxmint/muffin#410> + linuxmint/muffin#397
<linuxmint/muffin#397> + #8269
<#8269> + #8300
<#8300>. At least the first
issue might be a base branch cause, but at the same time could also be
Cinnamenu's actor hierarchy, so not sure but can test more this weekend,
along with some compositing benchmarks. When returning to Cinnamon without
this PR applied, I can only describe it as "much snappier".
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> - // ClutterEffect *effect = clutter_actor_get_effect (actor, "background-effect");
-
- // if (effect == NULL)
- // {
- // effect = st_background_effect_new ();
- // clutter_actor_add_effect_with_name (actor, "background-effect", effect);
- // }
-
- // const char *bumpmap_path = st_theme_node_get_background_bumpmap(theme_node);
-
- // g_object_set (effect,
- // "bumpmap",
- // bumpmap_path,
- // NULL);
+ {
+ if ((theme_node->background_blur > 0 || theme_node->background_bumpmap)
Might be good to do these logical comparisons when the change in CSS is
detected, instead of on every paint.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> @@ -278,6 +282,18 @@ st_widget_dispose (GObject *gobject)
st_widget_remove_transition (actor);
g_clear_pointer (&priv->label_actor, g_object_unref);
+ if (priv->background_blur_effect)
+ {
+ g_object_run_dispose (G_OBJECT (priv->background_blur_effect));
+ g_object_unref (priv->background_blur_effect);
+ priv->background_blur_effect = NULL;
+ }
+ if (priv->background_bumpmap_effect)
Indentation is off.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> @@ -1617,7 +1695,7 @@ st_widget_ensure_style (StWidget *widget)
g_return_if_fail (ST_IS_WIDGET (widget));
if (widget->priv->is_style_dirty)
- st_widget_recompute_style (widget, NULL);
+ st_widget_recompute_style (widget, NULL);
We use two spaces after a bracket-less if statement.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> @@ -486,8 +500,15 @@ st_widget_style_changed (StWidget *widget)
}
/* update the style only if we are mapped */
- if (clutter_actor_is_mapped (CLUTTER_ACTOR (widget)))
- st_widget_recompute_style (widget, old_theme_node);
+ if (clutter_actor_is_mapped (CLUTTER_ACTOR (widget))
+ || (old_theme_node != NULL &&
+ (old_theme_node->background_blur > 0
+ || old_theme_node->background_bumpmap != NULL)))
+ {
+ st_widget_recompute_style (widget, old_theme_node);
+
+ st_widget_add_background_effects(widget, old_theme_node);
Maybe we can check if the theme node is eligible before calling
st_widget_add_background_effects? We should know after
st_widget_recompute_style.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> @@ -1540,6 +1563,61 @@ on_transition_completed (StThemeNodeTransition *transition,
st_widget_remove_transition (widget);
}
+static void st_widget_add_background_effects (StWidget *widget,
+ StThemeNode *old_theme_node )
+{
+StThemeNode *new_theme_node = st_widget_get_theme_node (widget);
We could probably make st_widget_recompute_style return the new_theme_node
passed as an argument to this function and avoid a function invocation in
the paint cycle.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> + widget->priv->background_blur_effect = (StBackgroundBlurEffect *) st_background_blur_effect_new ();
+ widget->priv->background_blur_effect->blur_size = new_theme_node->background_blur;
+ for (int i=0;i<4;i++)
+ widget->priv->background_blur_effect->border_radius[i] = new_theme_node->border_radius[i];
+ }
+ }
+ if (new_theme_node->background_bumpmap != NULL)
+ {
+ if (widget->priv->background_bumpmap_effect == NULL)
+ {
+ const char *bumpmap_path;
+
+ widget->priv->background_bumpmap_effect = (StBackgroundBumpmapEffect *) st_background_bumpmap_effect_new ();
+ bumpmap_path = st_theme_node_get_background_bumpmap(new_theme_node);
+ widget->priv->background_bumpmap_effect->bumpmap_path = strdup (bumpmap_path);
+ for (int i=0;i<4;i++)
Spaces here will make the code easier to read.
------------------------------
In src/st/st-widget.c
<#8428 (comment)>:
> @@ -1540,6 +1563,61 @@ on_transition_completed (StThemeNodeTransition *transition,
st_widget_remove_transition (widget);
}
+static void st_widget_add_background_effects (StWidget *widget,
+ StThemeNode *old_theme_node )
+{
+StThemeNode *new_theme_node = st_widget_get_theme_node (widget);
+
+ if (old_theme_node)
+ {
+ if (old_theme_node->background_blur > 0)
+ {
+ if (widget->priv->background_blur_effect != NULL)
+ {
+ g_object_run_dispose (G_OBJECT (widget->priv->background_blur_effect));
In st_widget_recompute_style, we know if the style has *changed*, so it
might be good to use that boolean state instead of assuming any processing
is needed because there is an old theme node.
------------------------------
In src/st/st-background-effect.c
<#8428 (comment)>:
>
- }
- return TRUE;
- }
- else
- {
- return FALSE;
- }
+static gboolean
+st_background_blur_effect_pre_paint (ClutterEffect *effect)
+{
+ return TRUE;
What is the purpose of this function?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8428 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGB5MU6kjyVbON_otxX4HJ4R-yrJVQ3mks5vUIbXgaJpZM4bVM5_>
.
|
|
Hi guys, I'm prefixing it as WIP for the time being. |
clefebvre
changed the title
|
Just working through some of these now. Yes, it does show some odd things if you put it on .popup-menu-content. Not quite sure where .popup-menu-content fits in the processing for popups, but putting it here does not shapshot the screen background, perhaps something to do with the processing of the various layers in the menu. It also does not use the radiused corners of the popup as a whole, presumably because this element of CSS does not need to have matching radiused corners defined generally. It works fine in .menu though. |
brownsr
changed the title
brownsr
changed the title
clefebvre
changed the title
|
@clefebvre I'm not planning further work on this, as far as I am concerned it's been ready to pull since the last changes in March. |
brownsr
changed the title
|
Segfault during testing, so back to WIP. |
clefebvre
changed the title
|
Brownsr, any progress on this PR? |
|
Is this still being worked on? |
|
What an interesting feature. It is sad to see that the development stalled. |
|
if this isn't being worked on by @brownsr anymore, would it be fair for someone else to come in, take the proposed changes from this, and finish the implementation work? |
|
I would love to see this implemented into Cinnamon! Windows like the terminal would look so much better with transparency and blur. |
Is there information about what exactly had segfaulted? Is there a test case that's still reproducible? Does this need to be started over from scratch given how much time has passed? |
We're left with no answers. No way to reproduce, no info on repository, let alone what is required and what dependencies need to be taken care of, cinnamon compilation instructions, additional files or github links. Nada. |
|
god, i can't believe linux mint dev team bad¹ which, is a shame!! because cinnamon is a really cool desktop environment imo - it's a bit buggy, but it gets way less love than it deserves and xapps are like...the only competition to gnome apps i've found to be comfortable at all ( i really hope what happened to firefox/mozilla doesn't happen to linux mint, where the once gold standard loses its focus from bad management but noone has the resources to fork it properly and now they're making....5 built-in themes with bad text contrast, for a limited time? with all that donation money ( are last update's mint-y tweaks comparable to colorways? ) ) @rdlf4 so like, what do you suggest we (not me or anyone specific, people somewhat critical of mint in general) do about it? |
Alright, you're bringing a lot to the table here, so I feel like focusing on what's being discussed - in no way do I wish the Mint team to lose focus on supporting Cinnamon -- matter of fact, by restyling Cinnamon means supporting it even further, but on the visual style department. So give us Blur, and let us play around with it. Those with a beefier apu from a desktop computer can make it look prettier than those with a mobile apu, so having options to suit our needs and system settings is important. Don't get it twisted, I'm not saying "do it today! Or tomorrow! Or for the next release!" No, I'm asking the team to bring their attention to this topic and feature request, tell us how do-able it is with the current version of Cinnamon (does it allow for such a thing without compromising the Cinnamon team's agenda? Could it be done in an official way, meaning further supported for future Linux Mint releases?) - so some clarification from the team on this topic would be a nice place to start from. I'm sorry for the long post. |
|
@rdlf4 oh! yeah |
|
Merged via 6aeeb22 |
This reworks, enables, and extends the previous background effect facility that was turned off in the past as it was broken.
There are two possible background effects - bumpmap and blur. Both are turned on by CSS, and can be layered on each other if desired.
example CSS
Note that the size of the blur is treated as an iteration count. So background-blur: 1px is the basic effect, while background-blur: 2px applies the effect again on top of itself, and so on. The basic blur is relatively mild, twice is stronger, anything above 3 is overkill IMO.
Speeds seem fine with no noticeable speed penalty unless a large blur count is put in for test purposes.
The blur effect is probably the more useful. The bumpmap effect looks dramatic on a background, but rubbish over something like a text file being edited.
The effect works by snapshotting the screen underneath an actor, applying the effect to it, and then drawing it out as the lowest layer in the process of drawing an actor. There are no restrictions as to what can be layered on it, though clearly you won't want to put something without transparency over it or the effect would be hidden.
In the screenshots below the bumpmap is shown with nothing layered on it. The blur is shown with a semi-transparent background-gradient on top of it.
NB screenshots were taken before the masking code to allow rounded corners was added.