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

Fixes #372: calling size() in setup() producing weird behavior #423

Closed
wants to merge 2 commits into from

Conversation

procub3r
Copy link

@procub3r procub3r commented Feb 21, 2023

Polling for events within the size() function right after the size has been changed will make glfw trigger the window resize event right then and there.
This will create a new surface right after the call to size(), so that any graphics drawn from this point on in size() will be preserved.

Another issue still remains. The one where the buffers are not being swapped properly. Once a sketch is started, after a few seconds, the buffer being rendered to the window completely changes. After a few more seconds, the old buffer is rendered again, and this continues indefinitely.

I highly suspect that this is the same issue described by @tushar5526 here: #420 (comment)

As a result of this issue, the fix proposed in this PR will only be seen when this "buffer change" takes place. To confirm this:

  • Run an example program:
    from p5 import *
    
    def setup():
        size(512, 256)
        background(220)
    
    def draw():
        circle(mouse_x, mouse_y, 50)
    
    run(renderer='skia')
    
  • Wait for a few seconds. The first "buffer change" occurs. This is when you'll notice that the background(220) has taken effect, but in this other buffer.
  • And inevitably, the "buffer change" occurs again and you're back to the first buffer.

Demonstration:

bufferchangeglitch.mov

In the current master branch (without the fix in this PR), the background(220) call would not have taken effect in any buffer. I will be investigating the "buffer change" issue further.

Polling for events within the size() function right after the size has
been changed will make glfw trigger the window resize event right then
and there.
This will create a new surface right after the call to size(), so that
any graphics drawn from this point on in size() will be preserved.
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to p5py

@procub3r
Copy link
Author

This PR is not meant to be merged (yet). I just wanted to provide an example of what my fix would look like in code and also showcase this weird buffer change issue.

@procub3r procub3r changed the title Fix issue #372: calling size() in setup() producing weird behavior Fixes #372: calling size() in setup() producing weird behavior Feb 21, 2023
@tushar5526
Copy link
Member

I will test these changes, can you open an issue for the jittery behaviour?

@procub3r
Copy link
Author

Yup! I'll create it tomorrow (It's a bit late now lol)

@procub3r
Copy link
Author

Ah there's some tests failing cause I haven't taken vispy into account. Should be an easy enough fix

@tushar5526
Copy link
Member

Can you do the fix as well ?

Add a check and call poll_events() only if the current renderer is skia.
@procub3r
Copy link
Author

Added a check to call poll_events() only if the current renderer is skia. This gets rid of the error you get when you use the vispy renderer.

However, this simply eliminates the error when vispy is used. The behavior is not fixed for vispy. It doesn't seem as trivial to me right now because you don't control when the events are processed in vispy. It always happens after every timer "tick".

There might be a way to force an event poll before the next "tick" but a google search yielded no results.

@tushar5526
Copy link
Member

I checked this PR, but configuration in setup is not being persisted on calling size due to the same issue. I think #420 looks good to me and handles this pretty well. I will be closing this one, after merging that. Thank you for the efforts :D

@procub3r
Copy link
Author

procub3r commented Mar 2, 2023

Thats cool!
I built a lot of familiarity with the project working on this :D

@tushar5526
Copy link
Member

Thank you @procub3r, closing this PR.

@tushar5526 tushar5526 closed this Mar 3, 2023
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.

2 participants