-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Revert "Samplers: don't create unneeded empty samplers during startup" #12809
Conversation
This reverts commit cf9b481.
c5b56c9
to
daf5ead
Compare
I think in order to not require users to create expected decks and samplers explicitly in script files this could be done where control existence is checked in
|
ping Let's merge this in order to not release 2.4 with an annoying regression (primarily for user mappings, since I'm confident I fixed all built-in mappings in #12810). |
Oh, I am sorry this topic has put extra workload stress on you. I am not sure if merging this is a good idea. Do we hunt one regression (now only in custom mappings) with another, the performance regression and the reason we have created dedicated sampler skins? What is the effect when a mapping requests missing samplers? I think using a sampler without a GUI is unlikely. The assertion about the missing control is easy to fix. Probably more easy than a latency increase. After merging this, we have hard time to identify the not updated mappings. In general it is probably also questionable if mappings should be allowed to control samplers without a GUI. |
Never mind, it was my mistake to not do a smoke test with my regular setup. I'd have noticed right away. Tbh I didn't measure the performance implication of 64 unneeded samplers. Should be about more memory being used, not really CPU load (no sampler loaded --> no caching reader work, no engine buffer work IIUC).
The GUI samplers are created after the mapping has been loaded, that's the issue.
Not sure what you have in mind. Something like I expressed above #12809 (comment)
Hmm, not sure about this. Bottom line: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, than lets merge it. Thank you for explaining.
This reverts the core commit of #12657
Besides the Numark Mixtrack Pro3 #12808 other mappings slipped through in #12769.
I'm at it but I can't guarantuee to fix all of them in time.
Also, user mappings may be broken, too.
Let's push #12657 to 2.4.1
To summarize: the mapping control error can still occur if