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

S4 Mk3 screen support #13653

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

acolombier
Copy link
Member

@acolombier acolombier commented Sep 15, 2024

This PR include support for the S4 Mk3 screen support. It is made to work without #12199 although there is a setting to enable communication between mapping. This way, I can maintain a single mapping definition, without needing to get an API commitment for communication between mapping.

It includes two themes:

  • Traktor Pro stock inspired theme
    image
  • Joe Easton's inspired theme
    image

Depends on:

Here is the list of improvement made on the core mapping

  • Beatjump tab
  • Stem tab
  • Support for beatloop roll inside a loop
  • Screen feedback (optional, require Ability for controller to share data at runtime #12199 - temporary merge available on feat/s4-mk3-screen-support-with-shared-data)
  • Add library page scroll
  • Ability to preview the active deck hotcue
  • Ability change the hotcue color

@acolombier acolombier force-pushed the feat/s4-mk3-screen-support branch from 1c270cf to cdcbd4e Compare October 16, 2024 23:55
@acolombier
Copy link
Member Author

@mixxxdj/developers what are you opinion about where this PR is aiming?

I'm now at a stage where the mapping is pretty stable and usable. I appreciate that this introduce a huge amount of source code. I'm afraid there is no alternative, and I think the current screen content is really impressive! (Happy to post a full demo video if you would like me to justify adding 74 QML files...)

I would like to get an agreement in principle before I start working on the documentation PR.

Note that every C++ change is the early version of the dependencies and once those are merged, this PR should include no core source changes.

@ywwg
Copy link
Member

ywwg commented Oct 17, 2024

I will try to take a look at this soon.

@JoergAtGithub
Copy link
Member

Sure, we want to have this feature in Mixxx. This is why we co-financed the S4 Mk3 for Owen.
And as long as the code is device specific, we do not have the high requirements to the code anyway. These only apply for generic code shared between different controllers.

@ywwg
Copy link
Member

ywwg commented Oct 17, 2024

These only apply for generic code shared between different controllers.

the biggest point of controversy was how the data from the engine is shared with the controller. That is a mechanism that will be used by every controller with a screen, and needs to be safe and efficient. If we are agreed on how that works, then the rest of the changes are good to go

@acolombier
Copy link
Member Author

The approach I went with is a controller settings to enable the API rejected in #12199, is this is a must have for the screen.

As suggested in the PR description, this will allow me to maintain a single code base, and also allow advanced users to build this feature while still using the mainstream mapping.
If you've been following the development of the S4 on Zulip, you've probably noticed that I have been maintaining multiple forks and it has now become unmaintainable so getting it in one code base is a hard requirement on my end.

Obviously, if we ever manage to get a stable API in Mixxx, we can eventually make the migration.

@ywwg
Copy link
Member

ywwg commented Oct 18, 2024

for sure, we want to get this in and don't want to have forks that need maintenance. I do like the approach of using a controller setting to enable / disable the new API. That limits the blast radius nicely. If/when another controller screen comes online, we can iterate on the design as needed.

@acolombier acolombier force-pushed the feat/s4-mk3-screen-support branch from cdcbd4e to d3b16c3 Compare October 27, 2024 19:31
@acolombier acolombier mentioned this pull request Nov 17, 2024
8 tasks
@acolombier acolombier force-pushed the feat/s4-mk3-screen-support branch 3 times, most recently from f5b7c13 to 85f2705 Compare November 25, 2024 23:47
@acolombier
Copy link
Member Author

Manual PR available in mixxxdj/manual#703

@ronso0
Copy link
Member

ronso0 commented Dec 13, 2024

I tested feat/s4-mk3-screen-support-with-shared-data and I'm impressed! ❤️
(not sure if that is still en par with this branch?)
So many options, wow! Though, not everything is self-explanatory so some options are a bit hard to understand.

Some notes on the Advanced screen from a quick test:

  • I love that overlays can be adjusted (hidden, timeout)
  • remaining time is fixed, ie. not adjusted with tempo
    (time until next marker is scaled though)
  • what does "hide beatgrid" do? didn't notice a difference
  • Tempo Offset:
    when deck is sync leader (still "Master Deck")
    -> Tempo Offset is 100% with rate centered
    -> BPM Offset jumps between -0.00 and 0.00 when adjusting tempo
  • how can we activate the "browser mode"?
  • overview waves: cue markers like in the Classic screen would be nice
  • loop size marker is always visible
  • loop overlay timeout change is applied one second activation, not immediatly after Apply

rendergraph waveforms:

  • tearing noticeable for vertical lines (beatgrid, loop boundaries)
  • bit shaky, ie. amplitudes flicker when waveform scrolls
  • waveform gain is not applied(rendergraph issue I presume)

@acolombier
Copy link
Member Author

I'm impressed! ❤️

Thank you for this kind word. I have spent hundreds of hours on this feature, and with the recent backlashes I had on other efforts, I am very happy to see this one being well received. Really appreciated! :D

Though, not everything is self-explanatory so some options are a bit hard to understand.

Yes, I agree. I feel like some of it could use some more manual inputs, but I've been struggling to efficiently describe those options. There is also many settings for which I haven't provided a description at all yet.
Perhaps this is something we could iterate with? I know @ywwg also owns the device so perhaps we could divide and conquer on that one?

remaining time is fixed, ie. not adjusted with tempo

I will add the rate into the computation

Tempo Offset:
when deck is sync leader (still "Master Deck")
-> Tempo Offset is 100% with rate centered

Yeah, I'm tempted to disable that for now; I need to do quite some computation to help with this, same with the phase indicator. My plan was to add some RO COs to expose more of that information natively after the new grid system, but since this has taken a stop, I might simply remove it for now.

-> BPM Offset jumps between -0.00 and 0.00 when adjusting tempo

Sounds like a round bug. I'll have a look

how can we activate the "browser mode"?

Browser mode is currently not available (I'll hide the setting for it).
The reason being, it relies on the library to be implemented in QML, so it can be reused. For now, it relies one the base that exist in the new QML interface, but because the two models are incompatible, they cannot be used at the same time.
I'm hoping in the new year, we manage to make some progress on that front tho! I have done a lot of WIP on that front locally, and I believe @Holzhaus also have a few things WIP on his side.

overview waves: cue markers like in the Classic screen would be nice

Ah yes, I will add them back (they used to be there)

loop size marker is always visible

I cannot reproduce, are you sure that Keep the loop size indicator visible is set to false?

loop overlay timeout change is applied one second activation, not immediately after Apply

I couldn't reproduce either, but sounds like this could be related to a bug in in the controller setting (on blur event fired after apply event?)

what does "hide beatgrid" do? didn't notice a difference

This is still TODO, I was waiting for rendergraph for this, but this should come soon now. It will hide the grid on the waveform

tearing noticeable for vertical lines (beatgrid, loop boundaries)
bit shaky, ie. amplitudes flicker when waveform scrolls

Yeah, unfortunately, not much we can do about this. This comes to the limit of the S4 itself. The screen are very slow to be rendered (~75ms). During my reverse engineer, I realise you redraw part of the screen, so this is how I manage to unleash ~40FPS.
Unfortunately, the bottle neck remain how long it takes the device to draw the screen. During my discovery, I've also noticed you can send batch of updates, so I tried to emulate a "vertical sync" where I would draw bunch of vertical rectangle to reduce the tearing. The result was a weird waveform stretching, which felt even worse, but also the device would hard crash after 10min of usage (everything would freeze, including HID, Audio card and Bulk endpoint) so I decided not to let this as an option as it really felt like I was harming the device...

waveform gain is not applied(rendergraph issue I presume)

Yes, that's a limitation of rendergraph+QML due to the waveform factory monster

@ywwg
Copy link
Member

ywwg commented Dec 17, 2024

I will try to do some s4 hacking over xmas break!

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.

4 participants