Skip to content
This repository was archived by the owner on Dec 5, 2018. It is now read-only.

Fix for issue #92 #212

Open
jwdunn1 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix for issue #92 #212

jwdunn1 wants to merge 4 commits into from

Conversation

@jwdunn1
Copy link

@jwdunn1 jwdunn1 commented Oct 18, 2015

Adds Domiii's quick-fix, plus framerate control, and rAF fallback for older browsers. Tested.

@jwdunn1
quick-fix for issue processing-js#92
48563eb 
Adds Domiii's fix and also framerate support, along with rAF fallback for older browsers.
@jwdunn1
Merge pull request #1 from jwdunn1/jwdunn1-patch-1
8e025ee 
Fix for issue processing-js#92
Pomax
Pomax reviewed Oct 19, 2015
View reviewed changes
src/Processing.js Outdated
Copy link
Member

@Pomax Pomax Oct 19, 2015

Choose a reason for hiding this comment

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

don't leave old code in as a comment: the commit history takes care of making sure deleted code is easily restored.

@Pomax
Copy link
Member

Pomax commented Oct 19, 2015

Thanks for the PR, although this will probably need some runtime tests to verify that both loop and noLoop still work, and to verify that manually set frameRate values are still honoured (as best as possible).

Pomax
Pomax reviewed Oct 20, 2015
View reviewed changes
src/Processing.js Outdated
Copy link
Member

@Pomax Pomax Oct 20, 2015

Choose a reason for hiding this comment

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

probably want to remove this empty line

@GoToLoop
Copy link
Contributor

GoToLoop commented Oct 20, 2015

I hope every1 is aware that modifying the "Animation" loop is the most impactful & risky stuff there is.
Extensive tests are needed in order to make sure sketches don't turn out slower in performance!

Pomax
Pomax reviewed Oct 20, 2015
View reviewed changes
src/Processing.js Outdated
Copy link
Member

@Pomax Pomax Oct 20, 2015

Choose a reason for hiding this comment

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

While this makes sense from a timing perspective, having this scheduling happen first means that if there are errors in the code that gets called during onframestart, redraw, and/or onframeend, we keep trying to run it over and over. If we want to keep the timing stable, we'll need to a try/catch around the onframe/redraw/onframe block so that if there's a throw anywhere, we catch it, and at the very least set looping to false before throwing the exception on.

However, this function keeps running even if looping is set to false, because the scheduling happens prior to the loop check. That's probably going to cause unnecessary work for the browser, so it'll also be worth switching L4085 and L4086 so that we first check whether to do an early return because we shouldn't be looping, and then request the operation for the new frame, then process the current one.

@Pomax
Copy link
Member

Pomax commented Oct 20, 2015

@GoToLoop agreed

@jwdunn1
Copy link
Author

jwdunn1 commented Oct 20, 2015

FYI: The following simple sketch tests frame rates and loop start/stop. It begins at the default frame rate. A tap/click in each of the three zones of the square controls behavior as defined below.


void setup() {
  size(200, 200);
}
float x = 0;
void draw() {
  background(204); fill(127); noStroke();
  rect(67,0, 66,200); stroke(0); text(frameRate, 10,15); // display frame rate
  x = x + 1; if (x > width) x = 0;
  line(x, 0, x, height); 
}
void mousePressed() {
    loop();
    if (mouseX<66) noLoop();
    else if (mouseX>=67 && mouseX<133) frameRate(30);
    else frameRate(10);
  }

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jwdunn1 @Pomax @GoToLoop