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.36k stars 375 forks source link

Accessing per_pixel variable from per_point expression #64

Closed mbellew closed 2 years ago

mbellew commented 6 years ago

I added a bunch of assert()s to make sure I am understanding the parameter behavior. I found this behavior which seemed suspicious. In 'martin - reflections on the black tiles' there are these expressions

wave_0_per_point6=dx = dx.99 + xi; wave_0_per_point7=dy = dy.99 + yi;

'dx' on the rhs make sense, reading from the preset.dx parameter. On the lhs, is this is writing to the per_pixel matrix (which I would think would corrupt memory). What is the intended behavior here?

revmischa commented 6 years ago

i'm afraid i don't know. why would writing to the matrix corrupt memory?

mbellew commented 6 years ago

The matrix allocated for per_pixel parameters is a "float *" while memory allocated for per_point parameters is a "float ". I would think casting the "float *" to "float " and then writing there would trash some pointers. I may have to create a simple repro case and just step through this to see what's going on.

Still, regardless of the implementation, what does this expression mean? What does assigning to dx mean in a per_point expression? Maybe this preset is messed up, and this should have been flagged in the parser?

mbellew commented 6 years ago

Not surprisingly, my assert was wrong not the code. Here's the answer to my question. In this case "dx" gets parsed as a user defined variable, so it is separate from the preset dx variable. It's a little confusing at first glance, but consistent I guess.

mbellew commented 6 years ago

More subtlety: the first dx on the right hand side does refer to preset.dx, the assignment creates the user-defined param which is assigned to and is used for subsequent references.

BTW: I'm trying to figure this all out because in order generate code for JIT or compute texture, we need to understand the variables! This example has three variables named dx 1) preset.dx : the default frame/global singleton float readable/writable in init and per_frame code readable in per point/pixel eqn 2) per_pixel.dx : float[][] writable in the per_pixel equations 3) per_point.dx : user-defined float created implicitly in per_point equation read/write in per_point

mbellew commented 6 years ago

Actually, there is a bug here. The problem isn't on the assign side. As mentioned above, I see that the parser creates a user-defined param, so we're writing to that. However, dx can incorrectly read from the per_pixel mesh variable, rather than the preset output variable.

per_pixel_1=dx = dx 1.01 wave_0_per_point1=dx = dx.99;

I think this is a general problem with any point expression that reads a matrix variable that is set in a per pixel expression.

I think this is fixed in a branch I'm working on, but I wasn't planning on making a pull request any time soon. Maybe a fix would be to clear the matrix_flags after the per_pixel phase completes.

revmischa commented 6 years ago

Okay. Should per_pixel_1=dx = dx * 1.01 really be evaluated as multiplying itself? isn't that likely what the author intended? like dx *= 1.01?

mbellew commented 6 years ago

You're suggesting the user_defined.dx completely mask preset.dx? That could work. Either way we should probably have a warning for preset authors like the C compiler "user defined variable 'dx' masks read only variable 'dx'"

revmischa commented 6 years ago

I mean maybe they are trying to modify preset dx?

mbellew commented 6 years ago

As a side investigation I have this branch

https://github.com/mbellew/projectm/tree/assignment_expr

When a preset is loaded, it will print out a pseudo-code 'program' corresponding to the preset expressions. It's just to help me get my mind around the JIT/shader idea. It helps you see more explicitly what the expressions are doing than the original milkdrop format.

revmischa commented 6 years ago

Oh neat, what's the output look like? I've been digging into shaders more. I wonder if it'd be easier to emit GLSL or SPIR-V. SPIR-V might require porting everything to Vulkan? Or at least GLES? Need to learn more. Check out #26

mbellew commented 6 years ago

Still needs a little work, but here's a short example. I used "()" to indicate read-only. "preset.rad()" is not right, it should really be preset.rad(i,j) or maybe preset.points[i][k].rad().

function per_pixel(i,j) { preset.q1 = (0.5 sin( (preset.rad() 12) + (preset.time() 0.7))) + 0.5 preset.points[i][j].dx = (0.01 cos( (preset.time() 0.5) + preset.q1)) preset.points[i][j].dy = (0.01 sin( (preset.time() * 0.5) + preset.q1)) }

revmischa commented 6 years ago

neat

revmischa commented 5 years ago

This still an issue?

kblaschke commented 3 years ago

I'll keep this open as it's unclear and there are still numerous issues with waveform/equation rendering open.

IMO writing a good test suite with googletest for the whole parser and equation part would give us more confidence and assurance that it all works as intended.

kblaschke commented 2 years ago

I'll just close this issue, as there's a parser rewrite on the horizon which should fix most of these issues while adding missing equation features.

If there's a specific preset that doesn't work because values aren't transferred properly, please open a new issue with a specific example.