Fine tempo knob#178
Conversation
add FineTempoKnob to runtime_feature settings
added changes to enable FineTempoKnob coarse or fine default
|
Why is this change being put forward as the default? Is this something desired by most users? Is it likely to interfere with existing workflows? Personally, I would err on the side of the default being the previous, stock, expected behaviour rather than the new approach. (And to be clear, there's nothing wrong with the new approach, but it should be opt-in rather than opt-out) |
There was a problem hiding this comment.
The feature setting check would be cleaner if you compare it to the button press result instead of duplicating code
Also I agree with litui on the default value, I think this is better but until we have a good process for testing and reviewing UI changes it should be opt in
|
@litui I understand your point and I can change the default if it is an issue. This new behavior is the default on nearly every other machine and is my preferred default (also had a thread on the forums with people saying they'd prefer this behavior). But it's probably more consistent to keep the original default behavior so I will do that |
I'm not necessarily saying /don't/ make it the default, I'm just suggesting that if you're going to do something bold like making it the default you need to show your work. Link the forum thread for a start, and let's have a friendly debate on the merits of making it the default ;). Perhaps it's justified, but just putting in a PR with a changed functionality as the default doesn't reach the bar in my opinion. |
|
I will tinker and clean this up but any other thoughts/feedback are welcome... Also if there is any github protocol I am ignoring please let me know (gently, haha!)... Thanks for the feedback |
|
@litui Got it, no worries... I was just approaching it the same way I was with my own personal builds but I now understand that I need to think about all the other users not expecting a drastic change when they turn on their machines. This is all helpful feedback. The forum post I referred to was not quite as robust as I remember but it did get a few responses... https://forums.synthstrom.com/discussion/comment/22865#Comment_22865 I'll revisit this tomorrow after some sleep and research... |
|
Would it be possible to have settings for both tempo knobs states (turn and push+turn)? I ask because there's a third option I'd love to see of 0.1 BPM increments. If I had the choice I'd have turn change by 1 BPM and push+turn change by 0.1 BPM. We could add settings like:
Where:
Or just call SMAL "1" and FINE "0.1" of course. So we cold select which increment we want for both actions. |
|
That's a larger change and I think out of scope for this PR - this swaps the current behaviour, while your suggestion would require adding an entire new mode |
FineTempoKnob now defaults to OFF (can be enabled in runtime features menu
|
@m-m-adams Agreed 👍 I'll look at it when I can. |
I have cleaned up the code a bit (used 'switch' instead of 'if...else' for the main logic but had trouble trying to replace the 'runtimeFeatureSettings' line with a variable)
|
Hello everyone, thank you for all the feedback... I changed the default tempo behavior setting to the normal default (turn to increment by 4, push turn to increment by 1), tried to clean up the code a bit, but one piece of advice I wasn't able to get working was to store the 'runtimeFeatureSetting' value in a variable... The updated code works as expected though... Let me know if there are any issues with this update... |
|
Looks good! I just meant to check later, putting the check inline instead of a variable is totally valid |
removed unnecessary comments
|
I'm looking at the workflow conflicts and the changes in the latest community build and it looks like the approach for adding the runtime feature settings has totally changed. I can't really resolve these conflicts until I see how I need to implement this again... Looking through changes now and will update when I believe it is ready to review again. |
Update to the new way of using the community features menu
|
Sorry for the churn, I've sent a PR to your branch at suivaht#1 which should take care of the change. |
Update to new RuntimeFeatureSettings
|
@bobtwinkles the pull request was very much appreciated and has just been merged. Thank you |
|
Okay, I have been able to run the Formatting Check and Build workflow actions on my fork in my own repository, but I am not able to trigger the workflow actions here and I am not sure why... It passes both checks on my end... This is my first time going through this process so is there something preventing the workflow from running on this pull request? |
|
Gotcha covered, @suivaht. The checks have to be triggered by maintainers. |
|
@litui thank you... This is a great community... I appreciate all of the help from everyone who commented |
Followup for SynthstromAudible#178
Followup for #178
|
Hello all, very new to all this so apologies in advance, but I concur with weavermedia in respect to increments of 0.1. |
6 participants
Updated 7-12-2023
Just updating to clarify that this new feature defaults to OFF now...
Hello everyone, I'm trying this again... This build allows the user to switch the tempo knob behavior. New default behavior is that turning the tempo knob changes BPM in increments of 1. Pushing and turning the knob increments like the old default (typically by 4). This can be enabled/disabled in the runtime features menu (finally figured out how to do this after taking some good advice and checking out how @alter-alter added features to that menu... great community here). Please let me know your thoughts...