projectM-visualizer / projectm

projectM - Cross-platform Music Visualization Library. Open-source and Milkdrop-compatible.
https://discord.gg/mMrxAqaa3W
GNU Lesser General Public License v2.1
3.22k stars 364 forks source link

Integrate new preset parser and rewrite/refactor preset rendering. #716

Closed kblaschke closed 9 months ago

kblaschke commented 1 year ago

First of all, big sorry for creating such a huge PR.

Sadly, just replacing the preset and expression parser with the newly written one wasn't exactly easy, as the old code was highly integrated in basically all major effect classes. It worked on a few assumptions no longer compatible with the new expression compiler, so I had to reorganize code. This lead to the decision to reorganize the whole Milkdrop preset renderer into an overall better structure, removing the "ping pong" way of how the preset parts were rendered previously and replacing it with a more straightforward approach, effectively containing the rendering of a preset into a single function call.

Having to restructure classes, I've also went fully down into the rabbit hole and checked every rendering step for Milkdrop compatibility, fixed issues and added missing code. Also added framebuffer usage to the rendering process for various reasons listed below.

Changes

Here's a list of changes, improvements and other notable things contained in this PR, in no particular order:

Work left for later

Will fix or add the following features and bugs later, so we can get this huge changeset merged ASAP:

kblaschke commented 11 months ago

Emscripten version builds again, a crash was fixed and cleanups are done so far. I'd say the branch is in a good enough state to be merged to master, even if there's some work left to get it into a releasable state.

Please test extensively, review the changes and give your opinions.

JohannesKauffmann commented 11 months ago

First of all, big sorry for creating such a huge PR.

Forgive me if this may come across as bikeshedding, but some CMake parts of bf344b6b49adfcd3b5488759edd9622bfe9f96cd (the PROJECTM_STATIC_DEFINE parts) could be extracted to make this PR a bit smaller :)

kblaschke commented 9 months ago

Replaced the color masking with attaching/detaching the U/V texture for use with motion vectors, as GLES only supports per-buffer color masking with v3.2.

kblaschke commented 9 months ago

First of all, big sorry for creating such a huge PR.

Forgive me if this may come across as bikeshedding, but some CMake parts of bf344b6 (the PROJECTM_STATIC_DEFINE parts) could be extracted to make this PR a bit smaller :)

What exactly are you proposing there? Can't remember that I've done anything with the linking stuff, besides additing the evaluation library. Also unsure how this would make the PR substantially smaller, as the overwhelming part was rewriting code in the Renderer and MilkdropPreset[Factory] targets.

If we can make the static linking part easier, that's definitely a good thing to do. I'd do that separately though, as this PR already changes enough things, or better said, too many to even properly review. Could you create an issue please, detailing the proposed changes? This way I won't forget about that!

kblaschke commented 9 months ago

I'd personally say this changeset is as good as it gets without adding even more stuff to it, so I'd like anyone to pick up the branch, build and test it for stability. I've not seen any serious issues besides a few remaining rendering problems noted in the description, but these will be addressed later. Overall, it's a huge improvement over the current master, visually and performance-wise.

kblaschke commented 9 months ago

FYI, last push was a rebase on latest master to prepare for merging.

JohannesKauffmann commented 9 months ago

What exactly are you proposing there?

The commit A few Windows build fixes from this PR changed both CMakeLists.txt such that PROJECTM_STATIC_DEFINE is defined for a wider range/scope of targets. My proposal was to move these changes specifically to a separate commit/PR, as these CMakeLists.txt changes didn't seem related to the new preset parser/rendering rewrite, but would apply more globally. It'd make this PR a bit smaller (not much smaller admittedly).

Not sure it is worth changing now, however. PR looks good either way.

kblaschke commented 9 months ago

Ah, I see. Yes, I had to do this because otherwise the class definitions in the PCM class headers were different between the compile units, which the MSVC compiler doesn't like (different declspec attributes), causing linker errors. The MilkdropPreset target uses the PCM class to retrieve the frame audio data, and I didn't also want to refactor this class structure as it needs more attention later due to the broken/high spectrum and beat detection values. Could've done this in an earlier PR, but it's really just a small fix for a typo and adding missing defines to other targets. Proper cleanup will be done in issue #708.

kblaschke commented 9 months ago

Merging now, as the current branch state is stable enough for most uses. Emscripten might still have some issues left on top of the other things on the to-do list, so @Blaquewithaq please create an issue if there are any problems running on Emscripten, I'll tackle these in a later fix.