kivy-garden / frostedglass

FrostedGlass is a widget with translucent frosted glass effect, that creates a context with the background behind it.
MIT License
42 stars 6 forks source link

Critical bugs #8

Closed HeaTTheatR closed 1 year ago

HeaTTheatR commented 1 year ago

Hello! There are a few critical bugs that I found when using the FrostedGlass library. Let's look at a simple code example below:

from kivy.app import App
from kivy.lang import Builder
from kivy.metrics import dp
from kivy.uix.button import Button

from kivy_garden.frostedglass import FrostedGlass  # NOQA

KV = '''
ScreenManager:

    Screen:

        ScrollView:
            id: background_box

            GridLayout:
                id: box
                cols: 1
                size_hint_y: None
                height: self.minimum_height

        FrostedGlass:
            background: background_box
            size_hint: 1, None
            height: "56dp"
            blur_size: 15
            saturation: 1.3
            luminosity: 1.1
            noise_opacity: 0.0
            outline_color: "#388E3C00"
'''

class Example(App):
    def build(self):
        return Builder.load_string(KV)

    def on_start(self):
        for i in range(20):
            self.root.ids.box.add_widget(
                Button(
                    text=f"Item {i}",
                    size_hint_y=None,
                    height=dp(56),
                    on_release=lambda x, y=i: print(y),
                )
            )

Example().run()

First critical bug

If we try to press any of the buttons from the list on the screen, we will find that the buttons do not have the usual graphical response. Although button event bindings like on_press and on_release work.

No graphical response from buttons:

https://user-images.githubusercontent.com/16930280/226088599-9d230796-6631-4e1a-8916-9e5ae5dba438.mov

This problem is solved very simply. We need to update the canvas values every frame.

class FrostedGlass(FloatLayout):
    [...]

    def __init__(self, **kwargs):
        [...]
        Clock.schedule_interval(self.update_effect, 0)

This does not affect performance and FPS does not drop.

FPS with updating shader values every frame

https://user-images.githubusercontent.com/16930280/226089169-2fc8f88a-c4cc-4ed9-b0c0-45734b6f9ab2.mov

FPS without updating shader values every frame:

https://user-images.githubusercontent.com/16930280/226090302-508aa3f4-7f1f-466f-94ea-d1753460b76f.mov

Also updating the shader values every frame solves the above problem:

https://user-images.githubusercontent.com/16930280/226089274-d497e2de-9a46-4fc0-9bef-6145664a93bc.mov

Second critical bug

In the following methods, we use the isInstance method to determine the type of the object

    def on_parent(self, *args):
        [...]
        for p in self.parents_list:
            if isinstance(p, ModalView):
                [...]
            if isinstance(p, Screen):
                [...]
            if isinstance(p, ScrollView):
                [...]
        [...]
    def _bind_children_properties(self, children_list):
        [...]
        for widget in children_list:
            for property in properties_to_bind:
                try:
                    if property == "texture":
                        if isinstance(widget, Image):
                            [...]
                        if isinstance(widget, Video):
                            [...]
                    elif hasattr(widget, property):
                        [...]
                except Exception as e:
                    [...]
    def _bind_parent_properties(self, parents_list):
        for widget in parents_list:

            if isinstance(widget, ScrollView):
                [...]
            if isinstance(widget, ModalView):
                [...]
            elif isinstance(widget, Screen):
                [...]
            else:
                [...]
                except Exception:
                    [...]

It is not right. Because in this case, if the user will use custom classes, such as, for example:

class CustomScreen(Screen):
    [...]

Or:

class CustomScrollView(ScrollView):
    [...]

...expression in the above methods won't work. So instead of isinstance method we should use issubclass method. There are a number of other bugs that I found, but so far I have not had time to tell you how to fix them.

Thanks, I hope it was helpful!

DexerBR commented 1 year ago

@HeaTTheatR Thanks for the issue!

I'll still need to do some research before considering using the frame-by-frame update approach. I already have an idea of the reasons why this behavior is occurring.

I hope to be able to look into it soon.

In the meantime, feel free to share any other bugs you find. A minimal reproducible code and a description of the bug will be enough to help understand the cause of the bug.

Thank you again!

kengoon commented 1 year ago

I came across the same issue mentioned by @HeaTTheatR I had the same idea as him but in my case, I used refresh_effect to update the canvas on every frame using clock. Yes, it does not affect the FPS but it uses more CPU. my kivy app jumps from 2% CPU usage to 25% CPU usage when I update or refresh the frosted glass effect on every frame. To me, I think it is an issue.

Another way I tried solving this problem was to update or refresh the effect when a button is touched down (or starts rippling KivyMD) on every frame and then stop the effect update a second after the button has been touched up. This really worked but another issue rises when there is an infinite animation of a widget on a screen (ex: a button meant to be bouncing constantly). This means your clock has to run continuously to update the effect, which means more CPU usage.

suggestions I can offer are: 1) add on_touch_down and on_touch_up events to the list of properties_to_bind and handle them accordingly. maybe taking ideas from what I stated above. 2) watch out for animations. Detect if a widget is animating and run a refresh or update and then stop once the animation is stopped or the screen containing the animation is changed.

Thanks.

DexerBR commented 1 year ago

Hi @kengoon and @HeaTTheatR! Thank you for your feedback!

I just opened a PR to solve this issue, could you test it?

DexerBR commented 1 year ago

Second critical bug

In the following methods, we use the isInstance method to determine the type of the object

    def on_parent(self, *args):
        [...]
        for p in self.parents_list:
            if isinstance(p, ModalView):
                [...]
            if isinstance(p, Screen):
                [...]
            if isinstance(p, ScrollView):
                [...]
        [...]
    def _bind_children_properties(self, children_list):
        [...]
        for widget in children_list:
            for property in properties_to_bind:
                try:
                    if property == "texture":
                        if isinstance(widget, Image):
                            [...]
                        if isinstance(widget, Video):
                            [...]
                    elif hasattr(widget, property):
                        [...]
                except Exception as e:
                    [...]
    def _bind_parent_properties(self, parents_list):
        for widget in parents_list:

            if isinstance(widget, ScrollView):
                [...]
            if isinstance(widget, ModalView):
                [...]
            elif isinstance(widget, Screen):
                [...]
            else:
                [...]
                except Exception:
                    [...]

It is not right. Because in this case, if the user will use custom classes, such as, for example:

class CustomScreen(Screen):
    [...]

Or:

class CustomScrollView(ScrollView):
    [...]

...expression in the above methods won't work. So instead of isinstance method we should use issubclass method. There are a number of other bugs that I found, but so far I have not had time to tell you how to fix them.





@HeaTTheatR I'm not sure if the second issue is really a bug, automatic binding will parse object instances, so isinstance should work.

Note the example below:

class A:
    pass

class B:
    pass

class CustomA(A):
    pass

class CustomB(B):
    pass

class CustomAB(A, B):
    pass

custom_A = CustomA()
custom_AB = CustomAB()

print(
    isinstance(custom_A, A),
    isinstance(custom_AB, A),
    isinstance(custom_AB, B)
)

Output:

> True True True
HeaTTheatR commented 1 year ago

Hi @kengoon and @HeaTTheatR! Thank you for your feedback!

I just opened a PR to solve this issue, could you test it?

Yes, it works, thank you!

DexerBR commented 1 year ago

Fixed via #10