search

Tonejs / Tone.js

A Web Audio framework for making interactive music in the browser.
https://tonejs.github.io
MIT License
13.41k stars 976 forks source link

fix for AudioBufferSourceNode stop time miscalculation #942

Closed marcelblum closed 2 years ago

marcelblum commented 3 years ago

Fixes #912 by using now() in place of currentTime when calculating the stop time for an audio buffer player, to allow for edge cases with lookAhead and end offsets set.

tambien commented 2 years ago

thanks! pulling it in right now to see if it passes all the tests!

tambien commented 2 years ago

hmmm seems to cause some tests to fail. Would you have a chance to take a look at that? it'd be especially useful if you could write a unit test for #912 as well to make sure that this kind of error doesn't get re-introduced in the future.

marcelblum commented 2 years ago

Ok sorry I will take a closer look when I get a chance. I had built Tone locally and tested manually before committing and everything looked/sounded good but perhaps there were unintended side effects. Will post here if I have any trouble getting the test suite running locally. Hoping this wiki from 2018 is still current :)

marcelblum commented 2 years ago

Sorry to bother @tambien but I am running into several issues with the automated tests on Windows 10 (trying in both Powershell and MINGW64 Bash):

  1. Karma won't run at all unless I remove "type": "module" from Tone's package.json, otherwise it fails with require() of ES modules is not supported
  2. I am unable to do any of the batch multi test runs whether on directories e.g. npm run test --dir=core or on the whole project with just plain npm run test; it seems the problem stems from $npm_config_file and $npm_config_dir not being defined. I did find #545 from a couple years back but it's not clear what can be done to get batch testing working on Windows.
  3. Given that, I am forced to run tests on individual files as in npm run test --file=Oscillator which do work AFAICT, but obviously not a practical way to go about determining which specific tests my PR is causing to fail. Further, when I run for example that Oscillator test on just the upstream dev branch, it does not even pass. So it's doubly hard for me to determine which tests my PR was causing to fail if there were already failing tests to begin with... Or could there be some other quirk particular to running the tests on Windows that could cause tests that pass on your end to fail on mine?

I don't mean to open a can of worms wasting energy on fixing the larger issues with Tone tests on Windows, but if you have a quick fix for the $npm_config_file/$npm_config_dir issue I'm all ears!

Aside from that, just to get moving forward with this PR, if you could tell me which specific tests are failing due to my PR I could get working on it.

And just FYI here is the output when I run npm run test --file=Oscillator on the dev branch: Untitled

tambien commented 2 years ago

@marcelblum I spend some time last week getting karma to work on Node 14+. Have you pulled the latest?

The tests that failed are:

Pretty minor, might even just need a small tweak to those tests to account for the updated logic of the PR behavior.

marcelblum commented 2 years ago

@tambien you were right I wasn't using the very latest. I pulled the latest and redid npm install, and the require()-related errors are now gone 👍. But the $npm_config_file/$npm_config_dir issues remain preventing batch testing, and, more concerning, I'm increasingly finding tests that fail on Windows but pass on Mac (Player, GrainPlayer, Oscillator to name a few). It seems that running the tests on Windows is just plain unreliable (a lot of them maybe bc of platform-specific path stuff?) so for now I've dusted off my "unusable-keyboard-era" Macbook and will complete this work on there and submit a new pr soon.