Fix for unordinary behavior of size() in setup() for skia

[devAyushDubey]

This is a partial "fix" for #419 particularly for Skia Renderer. Related to #420

What I found tracing the calls are following:

• This issue is due to how GLFW handles window resize events.
• Whenever a glfw window is resized by size() or manually by the user, it triggers a window resize event in the glfw event queue, this event is inherently asynchronous, so this does not take place instantly, but when it does, a callback function is called. The callback function is assigned by self.assign_callbacks() in the start() of SkiaSketch.
• We assign frame_buffer_resize_callback_handler() as a callback for such events.
• In this callback, for the changed window size, we create a new surface with the new dimensions by self.create_surface(size=(width, height)).

Now here is the issue:

• This newly created surface which is clear and black is the new self.surface and is subsequently displayed on the window by the main_loop(), this is the reason why anything drawn in setup() with size() is not visible because a new surface is created above, everytime is size() is called.

Potential Fix:

• Re-render all the contents of setup() on the new surface.

This is exactly what I am doing here, I have done 2 changes:

• Called setup_method() again after the window is resized and new surface created, so that all the renders of the previous surface is again rendered on the new surface.
• Calling setup_method() again, posed the problem of recurrent calling of size(), to not let that happen, I have added a simple check in userspace.py/size() that ensures that, size() is not called in the re-rendering call.
• This check together with the added self.resized = False in frame_buffer_resize_callback_handler() also facilitates manual resizing of window and works perfectly fine.

Now

from p5 import *

def setup():
    size(512,512)
    background(220)
    circle(100,100,50)

run(renderer="skia")

With draw()

Manual Resizing:

[github-actions]

Thank you for contributing to p5py

[devAyushDubey]

Let me know your thoughts @tushar5526 🙂

[tushar5526]

This looks like a good solution. Thank for the deep dive. I will test this locally soon, but it seems calling setup inside the framebuffer callback every time will overwrite the current state of the sketch with the methods in setup.

In the second example, try adding a background(0) call in draw and resizing the sketch.

[tushar5526]

from p5 import *

def setup():
    size(512, 256)
    background(220)
    circle(100, 100, 50)
    stroke_weight(0)

def draw():
    stroke_weight(10)
    circle(mouse_x, mouse_y, 50)
    
run(renderer='skia')

Try this example and resize. See the stroke of the circle changes because setup is called again. Also, I am seeing a weird gittery behaviour at my end , some different frame buffer is getting swapped alternatively. Not sure if it is PR specific or different. It would take a proper deep dive though.

[procub3r]

Also, I am seeing a weird gittery behaviour at my end , some different frame buffer is getting swapped alternatively. Not sure if it is PR specific or different. It would take a proper deep dive though.

It's definitely not PR specific. I've noticed this behavior on the current master branch as well.
In fact, this issue prevents my proposal #372 (comment) from working all the time. It is definitely something worth looking into.

[devAyushDubey]

@tushar5526 I think you are right, there was this thing missing in my solution.

Calling setup_method() again in frame_buffer_resize_callback_handler() would mess up with the draw() logic of p5, as during a manual resize the draw_method() renders will be overshadowed by the setup_method() re-render call.

The problem:

• The problem with the solution was impulsively calling setup_method() and re-rendering it's content to the new surface without taking into account all the renders of the old surface.

Potential Solution:

• Taking into account the renders on the old surface and re-rendering all those renders to the new surface after the setup_method() in frame_buffer_resize_callback_handler().

I dived a little deep into GLFW and Skia and found out there is a way we can solve this.
I create an Image Snapshot of the old surface by calling makeImageSnapshot() on the old canvas which contains all the renders on that surface, we name it old_image.
After the new surface is created and setup_method() re-rendered, we render old_image on the new surface using drawImage() on the new surface's canvas.

I tried the examples that you mention, they seem to be working perfect on my end, it would be great if you could test it out too.

from p5 import *

def setup():
    size(512,512)
    background(220)
    circle(100,100,50)

def draw():

    circle(mouse_x,mouse_y,50)

run(renderer="skia")

With stroke_weight():

Let me know your thoughts. 🙂

[tushar5526]

The problem with calling setup() after resizing is that it will change the current style settings of the whole sketch. We need to prevent that from happening. The current solution will still not work. For example, we write a sketch where the background changes to red if we click the left mouse button, and we specify a black background in the setup. On resize, setup settings will be applied.

I think you are very close to the solution, but instead of calling setup every time frame resizes, we need to figure out a way to copy the existing verbs or commands that were sent to canvas and wait for the frame buffers to be resized - and then redraw or execute the commands, followed by continuing with the execution of setup. But, I think even p5.js is not able to handle resize properly and we should assume that users should put their size() calls at the top of the setup() functions. Further size calls in the draw function (say on mouse click) refreshing the whole buffer is fine.

PS: Following is not needed, but in case you want to go ahead one step.
Few other possible solutions could be to preprocess setup function to first execute size() wait for frame buffers to adjust and then draw other commands. (Not sure if we can do that in python, but worth a google search)

PS: I don't think we have to be strict with sketch refreshing completely to blank when we resize canvas manually as this happens in p5.js as well.

[tushar5526]

Also, I am seeing a weird gittery behaviour at my end , some different frame buffer is getting swapped alternatively. Not sure if it is PR specific or different. It would take a proper deep dive though.

It's definitely not PR specific. I've noticed this behavior on the current master branch as well. In fact, this issue prevents my proposal #372 (comment) from working all the time. It is definitely something worth looking into.

Yes, not sure if this is due to some glfw/skia updates or code bugs, but this was not the behaviour earlier and maybe this was introduced in some of the recent commits. Feel free to create an issue, I would love to help

[devAyushDubey]

I got your point @tushar5526 , we will have to retain our style settings if we are calling setup() in the callback.
I have implemented the same, I am using copy module to create a shallow copy of the style settings before calling setup() as old_style and assigning this old_style as our current style after calling setup() in the callback so that all the style changes of setup() can be discarded.
With respect to style, it would be like setup() was never called during the resize.

from p5 import *

def setup():
    size(512,512)
    background(100)
    no_stroke()
    circle(100,100,50)

def draw():
    circle(mouse_x,mouse_y,50)

def mouse_clicked():
    background('red')
    stroke(0)

run(renderer="skia")

Also I would be exploring if we can implement some sort of a preprocessing on setup() in python so that we can handle size() efficiently in future, for now I think this does the job.

Thank you for being so helpful and patient, do let me know if I am missing out on something 🙂.

[procub3r]

Nice fix!

[devAyushDubey]

Nice fix!

Thanks @procub3r 😃

[tushar5526]

Looks like a good hack. I think you should use deep copy instead of shallow copy. Otherwise changes in setup() will still be reflected in mutable style properties.

[devAyushDubey]

I implemented the required changes. Doing a deepcopy() now as it is more logical in our case.

[tushar5526]

Hey, I have tested this and this looks good to me. Can you add more verbose comments in the code explaining the logic and also mention the issue number in the callback.

[devAyushDubey]

Did the required changes.

[tushar5526]

LGTM! Thanks @devAyushDubey