Integrate new preset parser and rewrite/refactor preset rendering. #716
Conversation
kblaschke
force-pushed
the
rewrite-preset-parser
branch
3 times, most recently
from
648f8ca
to
b08a1ae
Compare
kblaschke
force-pushed
the
rewrite-preset-parser
branch
from
daf7bff
to
e4cda2b
Compare
kblaschke
changed the title
|
Emscripten version builds again, a crash was fixed and cleanups are done so far. Please test extensively, review the changes and give your opinions. |
| @@ -64,6 +60,28 @@ auto BeatDetect::GetPCMScale() const noexcept -> float | |||
| } | |||
|
|
|||
|
|
|||
| auto BeatDetect::GetFrameAudioData() const -> FrameAudioData | |||
There was a problem hiding this comment.
my knowledge of modern C++ is very poor. how does data get garbage-collected here? are we returning a struct copy? if so, since this is a hot path, can we return a pointer to a struct that frees itself when there are no more references or something? or is it not a big deal to copy 8 floats around?
There was a problem hiding this comment.
Yes, it's an object that's - in theory - copied. The compiler optimizes such return values as implicit moves, so when the code actually runs, no copy is created, but the object instance created in the function is moved to the caller's context without creating another object.
The audio data is only requested once per frame, and changes every frame, so using shared pointers here would just add overhead. The presets will get a const reference of this object, so ideally, only this single instance exists during a render call.
| -1.0, -1.0, 0.0, 0.0, | ||
| 1.0, -1.0, 1.0, 0.0, | ||
| -1.0, 1.0, 0.0, 1.0, | ||
| 1.0, 1.0, 1.0, 1.0}; |
There was a problem hiding this comment.
I had ChatGPT explain this to me
This Vertex Buffer Object (VBO) represents a square (technically, two triangles) that cover the entire screen in Normalized Device Coordinates (NDC), with texture coordinates associated with each vertex.
Here's a breakdown of the values:
The first two values in each row (-1.0, -1.0, 1.0, -1.0, -1.0, 1.0, 1.0, 1.0) represent the x and y coordinates of four vertices in a 2D space. This is a square that covers the entire viewport in NDC, where (-1, -1) is the bottom left corner and (1, 1) is the top right corner.
The third and fourth values in each row (0.0, 0.0, 1.0, 0.0, 0.0, 1.0, 1.0, 1.0) represent the texture coordinates (u and v) associated with each vertex. They range from (0, 0) at the bottom left to (1, 1) at the top right of the texture.
The drawing order of the vertices forms two triangles that together cover the entire viewport. The first triangle is formed by the vertices at (-1,-1), (1,-1), and (-1,1), and the second triangle is formed by the vertices at (-1,1), (1,-1), and (1,1).
There was a problem hiding this comment.
That's a very sophisticated explanation of "what is a quad?" 😅
Yeah, the blur textures are rendered as a simple quad, drawn with a specific gaussian blur shader. The quad also doesn't always fill the whole screen (or framebuffer, in this case), only in the first iteration. Subsequent blur steps will use only half or a quarter of the texture to create even more blurry images.
There was a problem hiding this comment.
yeah I just didn't get the texture coords part of it
| assert(renderContext.textureManager); | ||
| m_state.renderContext = renderContext; | ||
|
|
||
| // Initialize variables and code now we have a proper render state. |
There was a problem hiding this comment.
| // Initialize variables and code now we have a proper render state. | |
| // Initialize variables and code now that we have a proper render state. |
| public: | ||
| /** | ||
| * @brief LoadCode a MilkdropPreset by filename with input and output buffers specified. | ||
| * @param factory The factory class that created this preset instance. |
There was a problem hiding this comment.
| * @param factory The factory class that created this preset instance. |
| /** | ||
| * @brief LoadCode a MilkdropPreset from an input stream with input and output buffers specified. | ||
| * @param presetData an already initialized input stream to read the MilkdropPreset file from | ||
| * @param presetOutputs initialized and filled with data parsed from a MilkdropPreset |
There was a problem hiding this comment.
| * @param presetOutputs initialized and filled with data parsed from a MilkdropPreset |
| /** | ||
| * @brief Draws a flexible motion vector field. | ||
| * | ||
| * This is broken right now, as it only renders a relatively static 1px point grid, with no apparent motion trails. |
There was a problem hiding this comment.
No, not anymore. It's now properly implemented, I perform it purely on the GPU using a second color attachment and a vertex shader. Some presets like "Radar" and many others now actually look as they should.
Forgive me if this may come across as bikeshedding, but some CMake parts of bf344b6 (the |
|
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. |
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! |
|
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. |
Now rendering everything upside down to make up for the difference in UV coordinates in HLSL shaders.
Should fix visuals in about half of the presets.
Main issue was a wrong -, replaced with + in the rotation calculation. Plus we shouldn't negate the Y pos, as this may mess up UV calculations done in HLSL.
Idle preset doesn't render the same way as before, as it was presumably adapted to projectM's previous math. It's now a bit more complex, with the M tilting, a sine waveform at the bottom and some other tweaks. The filter mesh was broken, so the effects didn't render at all.
…oved ShaderEngine class. Milkdrop shaders are now read from .vert/.frag and .inc files, then copied into a generated code file by CMake. Makes editing easier, as many IDEs can properly syntax-highlight the shader code. The class was also moved into MilkdropPreset, as it's the only preset type which will ever make use of those shaders. Deleted the now fully "dissolved" ShaderEngine class.
… failed. Another option would be skipping the rendering if there's no active preset instead of displaying the idle preset.
…attachments and remove existing ones.
Only GLES 3.2 supports glColorMaski, which would exclude any platforms only supporting GLES 3.0 or 3.1, so we simply change the color attachment accordingly. This change also improves VRAM usage, as now there's only a single u/v texture used for both framebuffers as required.
kblaschke
force-pushed
the
rewrite-preset-parser
branch
from
1403e62
to
c08de98
Compare
|
FYI, last push was a rebase on latest master to prepare for merging. |
The commit Not sure it is worth changing now, however. PR looks good either way. |
|
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. |
|
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. |
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: