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

SlipMode waveform visual for RGB GLSL #13002

Merged
merged 4 commits into from
May 6, 2024

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Mar 25, 2024

This is an attempt to address the missing slip mode visual feature as requested in #8136

Other renderer will be addressed in different PRs.

image

TODO:

  • Render marker on the slip position part of the wave form upside down
  • Render the Post/pre roll

@JoergAtGithub
Copy link
Member

Did you consider to draw the whole waveform twice, half height stacked on each other?
I would use the indicator colors of the spinnies (white and orange) also on the waveform.

@acolombier
Copy link
Member Author

acolombier commented Mar 25, 2024

Did you consider to draw the whole waveform twice, half height stacked on each other?

That was my first thought, yes, but I quickly decided against as I thought synchronising animation between the two would probably be more work that extend the renderer with more visual abilities. I guess if we factor in the implementation on each renderers, it might turns out to be less work.

I would use the indicator colors of the spinnies (white and orange) also on the waveform.

Do you mean for the breathing animation around the slip point waveform? If yes, definitely agree: I've just hacked something together for now with a hardcoded ugly green, just wanted to gather some feedback before getting to work on something more polished :)

@acolombier acolombier force-pushed the feat/slipmode-render branch from 2b02cdf to 8a58627 Compare March 25, 2024 23:40
@m0dB
Copy link
Contributor

m0dB commented Mar 26, 2024

Nice that you are addressing this!

I had a hard time understand what was happening looking at the screen capture, and in the end I realised why: the start of track triangles are not part of the slip mode (which beat positions is). Is that intentional?

How about another approach: draw the slipping waveform on top, using waveformrenderersimple, so it is a single color, which 50% transparency.

Some comments (note, without looking at the code, so I don't know your current approach, it might very well be along the lines):

Assuming the rendering with the normal play position and the slip position is done in the same waveform widget (and I think it should), there is no need to do any synchronisation.

In waveformwidgetrenderer.cpp:

    double truePlayPos = m_visualPlayPosition->getAtNextVSync(vsyncThread);

you can call

    m_pVisualPlayPos->getPlaySlipAtNextVSync(vSyncThread, &truePlayPos, &slipPlayPos);

and you will get both positions synchronised.

The widget has a stack of renderers, and you can easily add more renderers to this stack to render the slip mode, and tell these renderers to use the slipPlayPos instead of the truePlayPos. When not in slip mode, you just skip all those renderers. You can then decide whether you want to only render the waveform itself for the slip mode, or (part of the) additional layers as well, or whether you want them to overlay with transparency or draw them tiled. In other words, it would equally allow to do what I am seeing in the screen capture, what Joerg is proposing and what I am proposing.

Apart from the synchronisation, using a single waveform widget with additional renderers makes sense because play mode and slip mode share a lot of information that is part of the widget (the waveform!), but also there is overhead in using a multiple widgets, as each has it's own QOpenGLWindow, so I would avoid that. (I have been thinking of using a single QOpenGLWindow for multiple WGLWidgets when possible: When multiple WGLWidgets combined occupy a single rectangle)

Let me know if this explanation is clear enough.

@acolombier
Copy link
Member Author

I had a hard time understand what was happening looking at the screen capture

That's my fault. I've tested with that dummy audio file I use to test waveform correctness (monotonous tone from 20Hhz to 20Khz).

the start of track triangles are not part of the slip mode

Yep I hadn't had implemented the pre/postroll triangle just yet. I just gave it a stab now (still incomplete with part of track missing on the slip waveform)

Kooha-2024-03-26-13-38-26.mp4

How about another approach: draw the slipping waveform on top, using waveformrenderersimple, so it is a single color, which 50% transparency.

Just to check I understood correctly, do you mean having it on top of the play waveform with transparency? I can think of two problem with this:

  • I might be hard to "recognise" the track with the section of track covering each other
  • (currently missing for the screen record but in my plans) The cues which will be displayed on the slip waveform are likely to also be hard to distinguish. We could make them look differently too (color, transparency) but things like label may also make the visual bloated

I guess it could be great to have renderer using different look for the slip visuals? Since it looks like renderer should implement this, I guess there is no technical limitation to make it look different in various waveform, so users may also gain more value out of using different renderers

you can call

    m_pVisualPlayPos->getPlaySlipAtNextVSync(vSyncThread, &truePlayPos, &slipPlayPos);

Cool, that's exactly what I'm using :)

The widget has a stack of renderers, and you can easily add more renderers to this stack to render the slip mode, and tell these renderers to use the slipPlayPos instead of the truePlayPos. When not in slip mode, you just skip all those renderers. You can then decide whether you want to only render the waveform itself for the slip mode, or (part of the) additional layers as well, or whether you want them to overlay with transparency or draw them tiled. In other words, it would equally allow to do what I am seeing in the screen capture, what Joerg is proposing and what I am proposing.

Sounds good. I will look at how easy to remove the currently hardcoded references to playPos and instead use an arbitrary pos for all the renderer

Let me know if this explanation is clear enough.

Thanks, that was very clear.

@acolombier acolombier force-pushed the feat/slipmode-render branch from 8a58627 to 28b49f9 Compare March 26, 2024 19:59
@acolombier
Copy link
Member Author

I just rewrote everything using your feedback and I think it looks much better, simpler and easier! Let me know your thoughts (visual available on Zulip)

@m0dB
Copy link
Contributor

m0dB commented Mar 26, 2024

Yes, this is the approach I had in mind! Nice!

@acolombier acolombier force-pushed the feat/slipmode-render branch from 28b49f9 to a17d8ec Compare March 26, 2024 23:02
@github-actions github-actions bot added the skins label Mar 26, 2024
@acolombier acolombier force-pushed the feat/slipmode-render branch from a17d8ec to 524eb44 Compare March 26, 2024 23:17
@acolombier
Copy link
Member Author

I would use the indicator colors of the spinnies (white and orange) also on the waveform.

I have set the same orange as the ghost spinny cursor for LateNight PaleMoon and red-ish for Classic, other themes default to a light grey, but happy to put some custom color if you have suggestion!

image

@acolombier acolombier marked this pull request as ready for review March 26, 2024 23:54
@ronso0
Copy link
Member

ronso0 commented Apr 7, 2024

Let me know your thoughts (visual available on Zulip)

This looks really good!

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this nice new feature. I have added some comments for improving readability.

WaveformRenderBeat::WaveformRenderBeat(WaveformWidgetRenderer* waveformWidget)
: WaveformRenderer(waveformWidget) {
WaveformRenderBeat::WaveformRenderBeat(WaveformWidgetRenderer* waveformWidget,
::WaveformRendererAbstract::Type type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is using the root namespace :: required here? If yes we should workaround this name clash.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it - WaveformRendererAbstract is defined both in src/waveform/renderers/waveformrendererabstract.h and src/waveform/renderers/allshader/waveformrendererabstract.h. Perhaps worth addressing the name conflict in another PR?

src/waveform/renderers/waveformrendererabstract.h Outdated Show resolved Hide resolved
src/waveform/renderers/waveformwidgetrenderer.cpp Outdated Show resolved Hide resolved
src/shaders/slipmodeshader.cpp Outdated Show resolved Hide resolved
@acolombier acolombier force-pushed the feat/slipmode-render branch from 115ea94 to a07530a Compare April 8, 2024 14:18
@acolombier acolombier requested a review from daschuer April 10, 2024 14:16
@daschuer
Copy link
Member

Unfortunately a conflict has been developed.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you.

@daschuer daschuer added this to the 2.5-beta milestone Apr 30, 2024
@acolombier
Copy link
Member Author

Conflict solved now

@daschuer
Copy link
Member

daschuer commented May 6, 2024

Thank you for this nice eye candy.

@daschuer daschuer merged commit 7d03a12 into mixxxdj:main May 6, 2024
13 checks passed
@fwcd
Copy link
Member

fwcd commented Jul 22, 2024

Love this feature! The only oddity I've noticed is that the markers also "shift" to the bottom half when using a waveform that doesn't support this split view in slip mode:

Screen.Recording.2024-07-22.at.17.15.47.mov

@acolombier
Copy link
Member Author

This is due the work we had started with @m0dB to mutualise all the allshader waveform - I will try to finish implementing the effect on the other waveform type now that it should be easier!

@acolombier
Copy link
Member Author

Created #13499 to keep track!

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

Successfully merging this pull request may close these issues.

6 participants