allshader RGB: Show a ghosted version of the original waveform behind… #12547
Conversation
… the gain-adjusted version. This was a feature of the legacy RGB waveform and allows the DJ to see frequencies that will be present if the EQ is changed.
|
from a technical note, this might be a good use for geometry shaders to generate the waveform mesh copy on the GPU instead... IMO for this to get merged it also needs to be configurable, as I can imagine that there is a sizable portion of people (me included) that doesn't like the look. |
|
I don't love adding settings for everything, I'd prefer to find visual solutions that are generally acceptably pleasant and useful. In this case, I'm restoring a feature in the legacy display that we've had for years and is just missing from the new one. What about the ghosts don't you like? |
I fully agree.
I didn't know about that. Good to know, though I doubt many actually people miss it (I haven't seen an issue about it yet).
I haven't tried this PR, but imagining how it will look, results in too much clutter. Moreover some people might simply not need the feature and would like to avoid the additional mental load induced by that extra visual noise. I also dislike that its a feature limited to only a single shader again, while with a little bit of clever rearranging it could be implemented generically for every shader. |
|
Independent of this specific feature (I still need to check it out), and in line with @Swiftb0y's comment, in my opinion the current approach with selecting a specific waveform renderer from a list is not ideal and I think a more option-based approach would make more sense. E.g.
and sure:
This of course would mean removing all the legacy waveforms, but that was the plan anyway. Once that is done, this should be pretty straightforward and would remove a lot of code duplication.
I kind of agree, and I do see that the flexibility and configurability of mixxx comes with it downsides (it slows us down, it increases the maintenance burden, it can cause confusion, it dilutes the developer effort), but I also think that it is what makes mixxx stand out in comparison with closed source applications, and it is an inherent aspect of an open source project. So IMO if we can come up with a default setting that is "generally acceptably pleasant and useful" while maintaining a degree of costumization, that would be ideal. |
| maxLow *= lowGain; | ||
| maxMid *= midGain; | ||
| maxHigh *= highGain; | ||
|
|
||
| // Calculate the squared magnitude of the gained maxLow, maxMid and maxHigh values | ||
| // We take the square root to get the magnitude below. | ||
| const float sumGained = math_pow2(maxLow) + math_pow2(maxMid) + math_pow2(maxHigh); | ||
| const float sumGained_ghost = math_pow2(ghostLow) + |
There was a problem hiding this comment.
You are overcomplicating things. You don't need to sum the low+mid+high components because ... (see below)
| @@ -164,6 +177,10 @@ void WaveformRendererRGB::paintGL() { | |||
| const float factor = std::sqrt(sumGained / sum); | |||
| maxAllChn[0] *= factor; | |||
| maxAllChn[1] *= factor; | |||
|
|
|||
| const float factor_ghost = std::sqrt(sumGained_ghost / sum); | |||
There was a problem hiding this comment.
... sumGained_ghost == sum, so factor_ghost is always 1.
| fpos + 0.5f, | ||
| halfBreadth + heightFactor * maxAllChn_ghost[1]); | ||
| m_colors_ghost.addForRectangle(red_ghost, green_ghost, blue_ghost, ghost_alpha); | ||
|
|
There was a problem hiding this comment.
Since the alpha doesn't change it would be better to use a uniform for that. It would be a simple to add the uniform for the alpha to the RGBShader, and that way you could use the same shader for normal and ghost.
| VertexData m_vertices; | ||
| RGBData m_colors; | ||
| VertexData m_vertices_ghost; | ||
| RGBAData m_colors_ghost; | ||
|
|
There was a problem hiding this comment.
The convention is to use camel case, not snake case.
|
@ywwg I think this should be targeting main. Are you planning to address the reviews? If you don't have time, I can do that too. |
|
yup I can repoint to main. I am back from travel so I will address the reviews this week |
|
Is this supersedded by #13151, or is this additional? |
|
we can close this one |
… the gain-adjusted version.
This was a feature of the legacy RGB waveform and allows the DJ to see frequencies that will be present if the EQ is changed.
this does not have to go in 2.4 -- strictly nice-to-have.