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

Add stacked glsl waveform renderer #3153

Merged
merged 4 commits into from
Dec 10, 2020
Merged

Conversation

poelzi
Copy link
Contributor

@poelzi poelzi commented Oct 5, 2020

This waveform is a filterd, stacked 3 band rgb waveform.
Each band is rendered upon each other.
The filtered part of signal is drawn in a darker shade, in case
of gain, the waveform is extended.
The waveform is then mixed with a rgb mixed component color to smoothen
the look and allow easier detection of a hi-hat.

Example:

Screenshot_20201005_231408

@ronso0
Copy link
Member

ronso0 commented Oct 5, 2020

As much as I understand the use case, do you find the difference distinct enough to suffice?
This is how it looks like here: for the left/upper track I killed all bands
image

@poelzi
Copy link
Contributor Author

poelzi commented Oct 5, 2020

It sure does not look like that on deer skin. I tried different renderers GL, GLSL and none of them work like I expect them to. And yes, I expect my GPU to do that, not my CPU.
As written, they are stacked. If you put +2db gain on bass, the mid and heights will move up and will not be shadowed by bass. If you put -2db on bass, the waveform stays the same but part of it is rendered darker. I can see where the bass is when filtered, at daylight, at night. This is what I expect, this is where every other waveform failed me.
You complained about adding two no settings so I wrote this new type of renderer. Additional code is minimal, the shader is not loaded so, I you don't like it, don't use it.

@ronso0
Copy link
Member

ronso0 commented Oct 5, 2020

Sure, if I don't like it I don't use it.
I'd just like to see an example of two tracks side by side with your favorite skin where I can see it works (and justifies the effort you've put into realising it :)

@poelzi
Copy link
Contributor Author

poelzi commented Oct 5, 2020

Screenshot_20201006_010514

Screenshot_20201006_010500

Screenshot_20201006_010443

Screenshot_20201006_010421

Comparision of RGB waveforms. You can clearly see, only mine is capable of showing where bass should be.

@poelzi
Copy link
Contributor Author

poelzi commented Oct 5, 2020

I'm working on using color pallets for the colors, so it will be nice to set it to whatever colors you like.

@ronso0
Copy link
Member

ronso0 commented Oct 6, 2020

To clarify the discussion here and in #3130: I am not complaining. Like others I am questioning the solution for a specific use case vs complexity for new users + the intransparency of the introduced settings/renderer combination as they clearly did only (visibly) affect a few renderers. (that new users may be overwhelmed by the amount of renderers is another story)

I described my 'outline idea' in https://bugs.launchpad.net/mixxx/+bug/1898721

@ronso0
Copy link
Member

ronso0 commented Oct 6, 2020

I'm working on using color pallets for the colors, so it will be nice to set it to whatever colors you like.

Nice.
This would basically be 3 color pickers below the 'Visual Gain' spinboxes, right?
and a checkbox to override the skin defaults and use custom colors?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 6, 2020

Nice, this looks like it's off to a good start. I agree with @ronso0 that it looks like there isn't enough of a visual difference when a band is turned all the way down on the EQ knob..

I'm working on using color pallets for the colors, so it will be nice to set it to whatever colors you like.

This seems excessive to me. IMO the waveform colors should either be the same across skins or slightly adjusted to fit in better with the skins' colors. I don't understand why we should complicate the preferences with this.

@poelzi
Copy link
Contributor Author

poelzi commented Oct 6, 2020

@Be-ing again, wtf. Why can we have color pallets for track colors and hotcues, but we will not give the user the possibility to change the waveform colors ?
Have you even looked at the code how those colors etc are parsed/transported/etc. It's a fucking mess. How is having multiple waveform instances just for different color settings as we have now any better ?
The waveform is one of the most important aspects of a dj program, why should we force colors on in this case ?

Then please, tell my all the settings I have to set, for getting a waveform that does what I described. I can't get any waveform behave the way this one does. If you don't understand it's behavior, please try the branch out. Play with the knobs, then compare it to the other waveforms, zoom in, zoom out.
I can't understand that you don't understand that stacking is something different then overdrawing.
We had a zulip discussion about this.
Do you think I waste my time writing a waveform that looks and behaves the same as another one ?
I have enough other branches to work on, but I did this because I have serious problems seeing what I want to see.

@ronso0 No, I will just add a new row in the colors tab that sets the pallette for the waveform. The waveform then just picks the first n colors out of there and uses them. I thought about 6, low, low filterend, mid,.... something like this.
I will create a pallette for RGB and then try some more designs.
There will be one special palette that uses the skin colors.

@ronso0
Copy link
Member

ronso0 commented Oct 6, 2020

Please let's all remember https://github.com/mixxxdj/mixxx/blob/master/CODE_OF_CONDUCT.md

@JoergAtGithub
Copy link
Member

My honestly opinion:

General:
-The stacked Waveform makes sense, because it shows information, other types don't
Regarding colors:
-Per default the waveform colors should match to the skin -> Best expirience for users who do not want to adjust settings
-But I see use cases to adjust them - e.g the different kinds of color blindness
Regarding the Waveform preference dialog:
-The layout looks unorganized which makes this dialog page difficult to understand
--But this is IMHO not due to the number of options itself
--The layout seperates not between Waveform and Waveform Overview (e.g. two columns next to each other would help)
--The sliders are far too long - if needed at all, sinces there are already numerical input fields
--Tooltips are missing

@Holzhaus
Copy link
Member

Holzhaus commented Oct 6, 2020

@poelzi Thank you, this looks useful. I didn't test this yet as I'm currently traveling, but as @ronso0 and @Be-ing pointed out I might be necessary to improve the contrast between the normal and the actual waveform a bit to make this even better.

Why can we have color pallets for track colors and hotcues, but we will not give the user the possibility to change the waveform colors ?

The original implementation of Hotcue colors had just a single fixed palette. This was changed for a specific reason: we allow importing libraries (including hotcues) from other DJ Software (i. e. Rekordbox, Serato). This means that you already use certain colors for your imported hotcues, and using different colors for hotcues set inside Mixxx would be confusing. Therefore you can use different hotcue color palettes to keep the colors consistent.

The same cannot be said about waveforms as we don't import them (even if we did, I doubt we can emulate other software's waveforms sufficiently to make them look consistent).

Have you even looked at the code how those colors etc are parsed/transported/etc. It's a fucking mess. How is having multiple waveform instances just for different color settings as we have now any better ?

If the parsing/data transfer code is bad, we can and should refactor it, but that's unrelated to the color palette feature.

Then please, tell my all the settings I have to set, for getting a waveform that does what I described. I can't get any waveform behave the way this one does. If you don't understand it's behavior, please try the branch out. Play with the knobs, then compare it to the other waveforms, zoom in, zoom out.
I can't understand that you don't understand that stacking is something different then overdrawing.
We had a zulip discussion about this.
Do you think I waste my time writing a waveform that looks and behaves the same as another one ?

Maybe you misunderstood @Be-ing's comment, but he didn't critize the basic idea of this new waveform, he just pointed out that the contrast between normal and actual waveform might be insufficient atm.

I will just add a new row in the colors tab that sets the pallette for the waveform. The waveform then just picks the first n colors out of there and uses them.

Please do not add the color palette stuff to this PR and keep it in a separate branch. That way the merge of either feature won't get blocked by pending discussions about the other feature.

I don't think it's a good idea to abuse hotcue color palettes for waveforms. It's confusing enough that track and hotcue color palettes are mixed (I'll probably open a PR to fix that when I'm back home). Let's not complicate it further.

TBH I don't think we should allow arbitrary, skin independent color palettes for waveforms because there is no real use case. If the colors are bad, we should fix it upstream to solve this for all users instead working around on a per-user basis by using a custom palette. Custom palettes could also lead to waveforms with colors that look out-of-place when changing the skin (so every time you change the skin, you'd need to edit the palette, too).

I can imagine we implement a combo box for choosing between, pre-defined settings, e. g. "Skin Default", "Monochrome", "High Contrast" "Deuteranopia", "Protanopia", "Tritanopia", etc. These can be defined by the skins to make them blend in nicely, but we can additionally provide some reasonable defaults to lower the barrier when implementing a custom skin. IMHO that would make it easier to use for visually impaired users.

@Holzhaus Holzhaus closed this Oct 6, 2020
@Holzhaus Holzhaus reopened this Oct 6, 2020
@Holzhaus
Copy link
Member

Holzhaus commented Oct 6, 2020

Sorry, I'm on mobile and didn't intend to close this. Apparently I have what we call "Sausage Fingers" in Germany ;-)

@poelzi
Copy link
Contributor Author

poelzi commented Oct 9, 2020

I have the color palette support for waveforms nearly finished. Only one bug left.

I decided that I will not touch the color code here because there is no sane way I can adjust the blue without affecting other subsystems and not hard coding the color in.
Better colors come with color palette support :)

@poelzi
Copy link
Contributor Author

poelzi commented Oct 9, 2020

@ronso0 I already implemented a bright outline. If you edit the shader and set drawBorder to true. I could not get it to look nice, maybe you will find some magic there :)
I implemented the color palette support currently in a way that allows you to specify all colors used in the waveforms. It pulls the colors from important to more unimportant ones and if not set, uses the normal skin colors. You can specify the filtered and unfiltered part separately.

@Holzhaus Holzhaus added the ui label Oct 13, 2020
@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:07
@Be-ing
Copy link
Contributor

Be-ing commented Oct 26, 2020

Is the mid band not getting drawn??? This is affecting the old GLSL renderer as well as the new one.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 4, 2020

@Be-ing fixed

@Be-ing
Copy link
Contributor

Be-ing commented Nov 5, 2020

Thank you for this innovative feature. I am glad that you put in the effort to come up with this solution rather than disabling the useful feature of adjusting the waveform with the EQs. I propose making this the default renderer (that can wait for another PR if it is controversial).

I still think there could be more of a visual contrast between a centered EQ knob and an EQ knob turned fully down. I don't think that should block this PR from merging though. We can continue to improve that in a follow up PR.

As far as I'm concerned, the biggest blocking issue for this is coming up with a more descriptive name. The current name gives no indication how it's different from the old RGB renderer. How about "RGB + shadow"?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 5, 2020

There are a handful of pre-commit issues. Please fix them.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 5, 2020

@Be-ing I will not address the coloring in this pr as I already have a branch that will bring color palettes to the waveform renderer. With some nice palettes for this, the waveform will look much better in future. The current colors are the same as they are used for the rgb overview and other renderers and would make those look strange.

@poelzi
Copy link
Contributor Author

poelzi commented Nov 5, 2020

I also implemented a border line where the normal form would be, but it's disabled by default and needs glsl changes. I could not get it look good only harsh. Maybe sombody with better shader skills can get this to work nice.
I will look if the mid could be adjusted without a hack. It could partially be the mix color that is calculated in, but I did this, because it helps to see kicks a lot.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 12, 2020

@poelzi have you thought about other names for this new renderer?

@poelzi
Copy link
Contributor Author

poelzi commented Nov 13, 2020

@Be-ing I'm open for a better name. 3band says what it does, shows the waveform in 3 distinguished bands.

This waveform is a filterd, stacked 3 band rgb waveform.
Each band is rendered upon each other.
The filtered part of signal is drawn in a darker shade, in case
of gain, the waveform is extended.
The waveform is then mixed with a rgb mixed component color to smoothen
the look and allow easier detection of a hi-hat.
@Be-ing
Copy link
Contributor

Be-ing commented Nov 13, 2020

Yes but the existing RGB renderers are also 3 bands. What name would clearly distinguish this from the older ones?

@Be-ing
Copy link
Contributor

Be-ing commented Dec 7, 2020

Ping. All that is missing to merge this is a descriptive, unambiguous name.

@poelzi
Copy link
Contributor Author

poelzi commented Dec 9, 2020

@Be-ing ready. I renamed the waveform to "RGB Stacked", that seems to be a common name for such waveform types.

@uklotzde
Copy link
Contributor

uklotzde commented Dec 9, 2020

I experience some flickering. But this is also present in Filtered (GLSL).

@poelzi
Copy link
Contributor Author

poelzi commented Dec 9, 2020

@uklotzde yes, especially when you move the beatgrid for example, heavy flickering in the glsl waveforms. In general, the VSYNC test seems to flicker here which means something more fundamental is broken ?

@Be-ing
Copy link
Contributor

Be-ing commented Dec 9, 2020

Thanks again for writing this!

The waveform rendering code is in need of a deep overhaul. https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/QOpenGLWidget.20migration/near/219036605

@poelzi poelzi changed the title Add 3band glsl waveform renderer Add stacked glsl waveform renderer Dec 9, 2020
@poelzi
Copy link
Contributor Author

poelzi commented Dec 9, 2020

@Be-ing done

@Be-ing Be-ing merged commit c444a86 into mixxxdj:main Dec 10, 2020
@poelzi
Copy link
Contributor Author

poelzi commented Dec 10, 2020

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants