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

Improved Winamp Visualizer by @x2nie #1260

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

Conversation

0x5066
Copy link
Contributor

@0x5066 0x5066 commented Aug 15, 2024

This contains work started by @x2nie and continued by me.

There's a lot that's either broken and/or unfinished.

The main goal of this PR is to improve the visualizer of Webamp, bringing improvements to the Oscilloscope and Spectrum analyzer rendering.

Improving the Oscilloscope was achieved by taking the rendering code from the Equalizer and implanting it into the small visualizer box done by x2nie, improving the Spectrum Analyzer involved ditching the Web Audio API in favor of FFTNullsoft, providing a much more accurate representation of the audio. The spectrum analyzer code (at least for the thicker bands) was redone that makes use of reverse engineered code from Winamp 2.65, more specifically how the bar chunking is achieved as well as how the peaks are calculated (this is broken).

This contains work started by @x2nie and continued by me.

There's a lot that's either broken and/or unfinished.

The main goal of this PR is to improve the visualizer of Webamp, bringing improvements to the Oscilloscope and Spectrum analyzer rendering.

Improving the Oscilloscope was achieved by taking the rendering code from the Equalizer and implanting it into the small visualizer box done by x2nie, improving the Spectrum Analyzer involved ditching the Web Audio API in favor of FFTNullsoft, providing a much more accurate representation of the audio.
The spectrum analyzer code (at least for the thicker bands) was redone that makes use of reverse engineered code from Winamp 2.65, more specifically how the bar chunking is achieved as well as how the peaks are calculated (this is broken).
Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for vigorous-lalande-5190c2 failed.

Name Link
🔨 Latest commit 920ee7a
🔍 Latest deploy log https://app.netlify.com/sites/vigorous-lalande-5190c2/deploys/66d0a5cddbefef00089cc895

@0x5066
Copy link
Contributor Author

0x5066 commented Aug 15, 2024

Dunno why it says it failed when GitHub says it passed all checks 🤷
Builds fine locally

@x2nie
Copy link
Contributor

x2nie commented Aug 15, 2024

0x5066 added 5 commits August 16, 2024 00:43
Various code comments explaining the purpose behind the new implementation
Peaks are no longer visible at the bottom
Replaced old Visualizer in the Playlist Editor in favor of the new Visualizer
…happens in paintFrameWide())

Also removed paintWavSolid() since that too is now processed in paintWavLine
Removed variables no longer in use
Adjusted how the FFT data is first processed in Vis.tsx and how that affects the thin and wide modes (they should now be consistent in volume)
…oublesize too!)

Proper color handling for the Spectrum Analyzer if in windowshade and double size mode
Removed comemnts/functions no longer in use
@0x5066 0x5066 marked this pull request as draft August 17, 2024 02:31
@0x5066 0x5066 marked this pull request as ready for review August 17, 2024 02:34
Fixed "doubled" not being able to be used outside of Vis.tsx
Set up base for eventual additional parameters that can be passed to the visualizer
Consolidate paintWavDot into paintWavLine
Remove dispose()
@0x5066
Copy link
Contributor Author

0x5066 commented Aug 17, 2024

I realized that this does make "dummyVizData" unusable, this can be worked around by disabling fft.timeToFrequencyDomain(in_wavedata, out_spectraldata); and filling out_spectraldata manually, though you will have to fill the 512 sized array completely, else you'll get an incomplete picture

Copy link
Owner

@captbaritone captbaritone left a comment

Choose a reason for hiding this comment

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

Overall I'm super excited about this! The Visualizer being "off" has always annoyed me about Webamp and it would be so nice get it much closer to the real thing!

Most of my comments are about style. I'd love it if anyone reading the code of Webamp found this code stylistically indistinguishable from the rest of the code (within reason).

In terms of correctness, my main concern is with the global variables that are shared between Webamp/visualizer instances. Happy to help brainstorm if you're not sure how to encapsulate them better.

Thanks so much for working on this! Would love to be able to get it merged!

packages/webamp/js/components/FFTNullsoft.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/FFTNullsoft.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/FFTNullsoft.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/FFTNullsoft.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/FFTNullsoft.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
0x5066 added 3 commits August 20, 2024 19:15
Attempt at addressing a few comments from the PR review
Finetuned the data going into timeToFrequencyDomain
Attempt at addressing more of the PR review
0x5066 added 4 commits August 27, 2024 01:17
Added checking for the state of the Main Window so that the Playlist Editor visualizer no longer bugs out like it did (incorrectly showing the small visualizer, or showing the full capacity of it)
Changed the global variable `i` to be `chunk` to avoid potential issues
Ensure `y` is scaled down (in the y axis) correctly when in windowshade mode
Skip rendering of the Oscilloscope in the windowshade mode to avoid it drawing out of bounds
Missed a few variables that werent in camelCase (again)
Prevent saPeaks from going "out of bounds" when the main window is in windowshade mode
…kground if Main Window was not visible, in windowshade mode and not in double size
Copy link
Owner

@captbaritone captbaritone left a comment

Choose a reason for hiding this comment

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

Sorry for the longer list of comments. I didn't get too into the nitty gritty of the visualizer algorithms but I figured I'd leave what I have for you now.

Thanks again for your work on this!

packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/Vis.tsx Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
packages/webamp/js/components/VisPainter.ts Outdated Show resolved Hide resolved
0x5066 added 2 commits August 29, 2024 18:00
Fixes FFT being corrupted when multiple instances of Webamp exist and are playing at the same time
Fixes multiple Webamp instances fighting over what the current state of the Main Window really is
Moved a lot of global mutable variables to instead be owned by BarPaintHandler and PaintWavHandler
Renamed visualizer functions since they now handle a lot of things
@0x5066 0x5066 requested a review from captbaritone September 1, 2024 13:20
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.

3 participants