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

Bug fix for frameRate() #6015

Merged
merged 3 commits into from
Feb 26, 2023
Merged

Bug fix for frameRate() #6015

merged 3 commits into from
Feb 26, 2023

Conversation

quinton-ashley
Copy link
Contributor

@quinton-ashley quinton-ashley commented Feb 13, 2023

My first pr to p5.js! 🎊 This pr doesn't provide a solution to issue #6013 or add any new functionality, it just fixes a bug I found.

I saw that the _frameRate and deltaTime values were calculated after redraw() but they should be calculated before redraw()

I also avoided subtracting the same values twice by putting the deltaTime calculation first.

See @davepagurek 's example sketches to see the fix in action.

Ball's speed doesn't not match when using frameRate() vs millis():

https://editor.p5js.org/davepagurek/sketches/K7cKw7bie

With the fix the balls' speeds match:

https://editor.p5js.org/davepagurek/sketches/VXHoBahkN

@welcome
Copy link

welcome bot commented Feb 13, 2023

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@davepagurek
Copy link
Contributor

Thanks for making a PR for this! Before I review it more I need to do a bit more testing to make sure there aren't any other bits of code that would need to change too. I haven't combed through the code to look for anything yet but I'll keep discussing in #6013 when I do, and then I'll get back to this PR!

The touches array is supported on Safari for iOS. There are no touch screen Macs.
Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

I finally got around to looking into what parts of p5 use frameRate and it seems like this change is safe!

I think we can merge the src/core/main.js changes, but would you mind moving the other docs change out of this one? Our usual Github flow, to help us track changes better, is to open an issue first, and then make a PR for the issue after there's been some discussion and a maintainer gives the go-ahead.

src/events/touch.js Outdated Show resolved Hide resolved
@quinton-ashley
Copy link
Contributor Author

@davepagurek I think it worked, it says files changed (1) at the top now. You can merge now!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

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

looks good, thanks @quinton-ashley!

@davepagurek davepagurek merged commit 7bdb6ec into processing:main Feb 26, 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