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

Wrap drawing in requestAnimationFrame (multicanvas). #1106

Merged
merged 12 commits into from
Jun 11, 2017
Merged

Wrap drawing in requestAnimationFrame (multicanvas). #1106

merged 12 commits into from
Jun 11, 2017

Conversation

agamemnus
Copy link
Contributor

@agamemnus agamemnus commented May 27, 2017

Wrap drawing in requestAnimationFrame (multicanvas) per #1084.

Blocked by #1107.

return;
} else {
peaks = channels[0];
var requestAnimationFrame = window.requestAnimationFrame || window.webkitRequestAnimationFrame;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add support for opera and IE? … Maybe this would be something to put in utils, since it is also used in MultiCanvas and potentially other places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like this: https://gist.github.com/mrdoob/838785

@agamemnus
Copy link
Contributor Author

agamemnus commented May 27, 2017 via email

@agamemnus
Copy link
Contributor Author

I don't know what's going on here.


drawLine: function (peaks, absmax, halfH, offsetY, start, end) {
drawLine: utils.frame(function(peaks, absmax, halfH, offsetY, start, end) {
Copy link
Owner

Choose a reason for hiding this comment

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

utils is undefined. It has to be WaveSurfer.util. Please also restore the spaces after the function keyword.

@agamemnus
Copy link
Contributor Author

It's a function call though. Tough call..

@agamemnus
Copy link
Contributor Author

Also I wrapped drawLine by accident instead of the one above it (or so). My bad.

@thijstriemstra
Copy link
Contributor

anyone an idea what this error is about? https://travis-ci.org/katspaugh/wavesurfer.js/builds/237172952#L352

@agamemnus
Copy link
Contributor Author

Yeah... it's because I don't have all the requestAnimationFrame PRs in one place, so it doesn't know about util.frame...

@thijstriemstra
Copy link
Contributor

You'll probably have to bring in more code in this PR so the travis build doesn't fail, or find a way to make travis build happy.

@agamemnus
Copy link
Contributor Author

Once the util PR is accepted, this one will work.

@thijstriemstra
Copy link
Contributor

what pr number?

@agamemnus
Copy link
Contributor Author

The one you pinged me about 20 min ago...! :P

#1107

@thijstriemstra
Copy link
Contributor

thijstriemstra commented May 31, 2017

cool, just for reference so others don't have to dig around for the PR nr. You should probably add to the PR description something like "blocked by PR #" (for every relevant PR you made/make).

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Jun 5, 2017

Tests fail with:

TypeError: Illegal invocation

	    at Object.drawWave (dist/wavesurfer.min.js:4:9038)

	    at Object.t.Drawer.drawPeaks (dist/wavesurfer.min.js:4:21459)

	    at Object.t.empty (dist/wavesurfer.min.js:4:8462)

	    at Object.t.load (dist/wavesurfer.min.js:4:5975)

	    at Object.<anonymous> (spec/wavesurfer.spec.js:23:20)

so this type of method definition isn't allowed it seems..

@agamemnus
Copy link
Contributor Author

It's completely unclear what is going on here. The same changes worked in the Canvas PR.

@katspaugh
Copy link
Owner

Weird. Can you reproduce this error locally?

@agamemnus
Copy link
Contributor Author

Found the issue. See: #1120.

@katspaugh katspaugh merged commit d9f3394 into katspaugh:master Jun 11, 2017
@katspaugh
Copy link
Owner

The tests pass now, thanks!

mspae pushed a commit to mspae/wavesurfer.js that referenced this pull request Aug 18, 2017
* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* I have no idea why this failed.

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js
mspae pushed a commit that referenced this pull request Aug 19, 2017
* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* I have no idea why this failed.

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js

* Update drawer.multicanvas.js
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.

None yet

4 participants