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

Regression: Save no longer works #47

Closed
nulldg opened this issue Oct 14, 2024 · 9 comments
Closed

Regression: Save no longer works #47

nulldg opened this issue Oct 14, 2024 · 9 comments

Comments

@nulldg
Copy link

nulldg commented Oct 14, 2024

exporting bytebeat songs used to work, but it had been broken at some point and now crashes.

Uncaught (in promise) TypeError: (intermediate value).createContext is not a function
    realSave https://greggman.com/downloads/examples/html5bytebeat/editor/index.js:530
    save https://greggman.com/downloads/examples/html5bytebeat/editor/index.js:556
    showSaveDialog https://greggman.com/downloads/examples/html5bytebeat/editor/index.js:570
    startSave https://greggman.com/downloads/examples/html5bytebeat/editor/index.js:483
    setElemProps https://greggman.com/downloads/examples/html5bytebeat/editor/elem.js:5
    createElem https://greggman.com/downloads/examples/html5bytebeat/editor/elem.js:24
    main https://greggman.com/downloads/examples/html5bytebeat/editor/index.js:343

calls are made to ByteBeatNode.createContext and ByteBeatNode.createStack in index.js. while ByteBeatNode exists and these functions exist inside the prototype, debugger reports that the functions are undefined at the exception site, which is unsurprising because they are instance functions being called like static functions.

another call is made to getSampleForTime from ByteBeatNode, which appears to have been removed in b28f455, so this may also crash.

@nulldg nulldg changed the title Save has stopped working Regression: Save no longer works Oct 14, 2024
@nulldg
Copy link
Author

nulldg commented Oct 14, 2024

just confirmed locally that 64b842d was the last commit where the feature worked, and that the regression appeared in b28f455. the function must simply have been forgotten about when moving user code processing to a worklet. it's an unfortunately easy mistake to make in javascript.

@greggman
Copy link
Owner

it's an unfortunately easy mistake to make in javascript.

No idea what it has to do with JavaScript. I've seen regressions in every programming language ever.

In any case it should be fixed. You probably need to clear your cache.

@nulldg
Copy link
Author

nulldg commented Oct 15, 2024

oh wow, that was extremely fast! thank you so much!

No idea what it has to do with JavaScript. I've seen regressions in every programming language ever.

moving/deleting functions without updating every caller causes compilation errors in statically-typed early-binding languages so it's essentially impossible to miss these broken call sites, but in javascript, the program runs correctly until the broken code is executed, making it very easy to miss these types of regressions when moving stuff around.

@nulldg
Copy link
Author

nulldg commented Oct 15, 2024

addressing the commit message:

I don't know enough about .wav format to know why the result is softer than the original.

when html5bytebeat plays the song, it gets upsampled using nearest-neighbor sampling. when it exports a song, it outputs audio exactly as generated and at the song's sample rate, leaving upsampling up to the playback applications, which often try to use something like libsamplerate or speex to resample the audio - these unfortunately filter the high frequencies out so it sounds muffled and soft.

if it's an issue, you could make html5bytebeat upsample to 44.1kHz during export, or you could leave it as-is and just ask users to change their playback settings

@nulldg
Copy link
Author

nulldg commented Oct 15, 2024

anyway, again, thank you so much, greggman!!

@greggman
Copy link
Owner

moving/deleting functions without updating every caller causes compilation errors in statically-typed early-binding languages so it's essentially impossible to miss these broken call sites, but in javascript, the program runs correctly until the broken code is executed, making it very easy to miss these types of regressions when moving stuff around.

And yet statically typed languages still have regressions all the time. I work in C++ and Rust and the same types of bugs still happen often. Someone makes a change and it breaks something that was workkng.

@nulldg
Copy link
Author

nulldg commented Oct 16, 2024

i literally never said regressions don't happen in other languages. i'm talking about this specific bug, where you move a function and forget to update one of the call sites.

@nulldg
Copy link
Author

nulldg commented Oct 16, 2024

all i was saying was that forgetting to update realSave in b28f455 was completely understandable and i would have made the same mistake! i sometimes rely on the compiler telling me that i've invalidated some random call site by moving or deleting a function it depends on.

@nulldg
Copy link
Author

nulldg commented Oct 16, 2024

really sorry for the misunderstanding.

anyway, i've tested production under a bunch of different cases and the feature seems to be completely functional again, working just like before 🎉

thank you again greggman, you're a legend!

@nulldg nulldg closed this as completed Oct 16, 2024
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

No branches or pull requests

2 participants