-
Notifications
You must be signed in to change notification settings - Fork 86
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 Pamela's "EuroPi" Workout #248
Conversation
I haven't noticed any bugs yet but I do have a small suggestion: I've never used a real Pams, so I have no idea if this is intuitive if not, but without using this for a long time I keep forgetting what the button does, and it seems like on every screen there's space for this, so I'd like to suggest a little text that says 'edit' or 'back' depending on if you're currently editing a value
Please say if you think this is an unnecessary addition, but it has at least personally helped me use the script without ever pressing the wrong thing, but before I added it I quite often would pause when I meant to go back out of edit mode |
It looks like there was a bad merge in there somewhere and there are files included in this PR that are related to other changes outside of this PR. Would you mind re-syncing your branch with HEAD and fixing the extra files? |
…s (except the bad merge)
I just updated my fork's main and re-applied the changes. Most of the old commit history is gone, but I'm not too worried about losing that; there were some dead ends that can be left in the past. |
I'm using inverted text to indicate that you're currently editing, and normal text to indicate a read-only value. I thought that would be clear enough, but maybe it isn't? I'm not sure that's necessary? The left button should always be the start/stop button. The edit/apply/back button is always the one on the right. A long-press on B2 will always cancel your current edit and flip back to the other menu level. On the ALM module, the run/stop button is a big yellow button, and the select/edit/back button is the "in" action on a clicky encoder knob. I tried to replicate that as best I could by using the left button as the run/stop and the right button as the selection. |
…ng undesired consequences when using the script as a S&H quantizer. This simplifies the setup and prevents accidental (and sometimes dramatic) pitch-shifting as a result of the default settings
…s! Woo! Can't believe I didn't think to add that earlier...
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.
Wow this is a heroic effort. Thank you for it. The script itself is amongst the more complex that we have, but it is well factored such that it is pretty easy to navigate and understand. Bravo.
I've reviewed most of the code and played with the UI some. I'd like to spend some more time on it, but thought that I would send my first batch of comments over to you.
Also, thank you for being patient with the time it has taken for someone to provide feedback, as well as whatever may fall out of issue #275.
software/contrib/euclid.py
Outdated
@@ -62,6 +64,8 @@ def generate_euclidean_pattern(steps, pulses, rot=0): | |||
raise ValueError | |||
if steps == 0: | |||
return [] | |||
if pulses == 0: | |||
return [0]*steps |
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.
[question] What's going on here? This change seems unrelated and undocumented.
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.
The commit message for that change got lost in a bad rebase. This change fixes a bug where if you tried generating a euclidean rhythm with N steps and zero pulses the function crashed with a division-by-zero error. So instead if pulses
is zero just return [0, 0, ...., 0]
indicating a euclidean rhythm of length N where all steps are off.
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.
Thanks for the explanation. In the future it would be ideal to have but fixes for separate scripts in their own PR. Aside from reducing the general scope of this PR, the bug fix would likely have been able to be merged already.
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.
The bug only became apparent when implementing Pam's euclidean generation, which is why I just fixed it in this branch. The Euclidean script itself works in its current state.
Depending how long this PR takes I can cherry-pick that fix into a separate merge request though.
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.
Thanks for the explanation. It now makes sense to me for this change to be in this PR. I don't think you need to change anything.
## Menu Navigation | ||
|
||
Rotate `k1` to scroll through the current menu. Pressing and holding `b2` for 0.5s will | ||
enter a sub-menu. Pressing and holding `b2` again will return to the parent menu. |
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.
[discussion] I found myself getting lost a little bit in the menu operation. It might help to provide some sort of visual indication of what K1 was currently controlling, the top menu or the sub menu. I think that something like underlining or inverting the text could work.
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.
I'm not sure I like inverting or underlining, since if anything ever has 3 layers of menus it would be nice to have a consistent look & feel.
I'll see if I can add some sort of depth icon in one of the corners, similar to the current play/pause button in the upper-right.
I'm not sure a visual indicator of whether you're on the top-level or sub-menu would necessarily be useful though; unless you know what options are in which level knowing the level you're in doesn't actually help that much. And if you do know what options are in which level, the visual indicator probably isn't necessary.
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.
Since I was making menu changes anyway I've added a label to the lower-right corner of the submenu screens. e.g. if you're in the submenu for CV2 you'll see cv2
in the lower-right. This should hopefully reduce the ambiguity.
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.
Sorry, I don't think that my suggestion was very clear, and I find the new label even more confusing. I'll try to explain again.
When you are navigating the menu, the K1 controls the selection of the menu item, and K@ controls the actual value of the setting. When K2 is actually going to be able to change the value of the setting you invert the text. This give a visual indication of what you are interacting with. I think it works quite well.
I'm suggesting that you do a similar thing with K1 and the menu levels. So If we were at the top of the menu and K1 would select amongst the CV outputs, that part of the display would be inverted. After you long press to go to the next level of menu, the text inversion would change to indicate it.
This should work fine with any number of menu levels and would be consistent with the behavior you are already using of having the field that will be changed identified with inverted text.
I didn't follow your reasoning about knowing what options are available.
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.
I think I understand what you mean now. Try the latest revision and see if that makes more sense to you.
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.
Yes, this is exactly what I was thinking of. i think that it's a lot more clear now. I might put a little more space between the two items (or maybe some kind of separator) if you have the room, but it's not critical.
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.
I can try to tidy up the formatting a little bit. Shouldn't be too hard to add a little bit of spacing between the two parts.
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.
I tweaked the spacing a little more in the latest revision. Between the inverted text and a little more room between the prefix and title I'm not sure it needs a symbol to separate them. Give it a shot and let me know what you think. I might add a little more space, but I like how it currently looks about the same width as a single space between words.
selected_index = k2.choice(list(range(len(self.setting)))) | ||
self.setting.choose(selected_index) | ||
else: | ||
self.set_editable(True) |
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.
[optional] The value of the setting jumps to the current value of the knob when the setting becomes editable, which can be shocking for the user. Consider using an expiremental.KnobBank
to alleviate this.
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.
Yeah, that's a mild frustration I've had with the menu too. It would be nice if there was an encoder instead of one of the knobs, but alas.
I'll look into the KnobBank class, but it likely won't get changed for a while.
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.
KnobBank is the better way to go, but one way I've addressed this is having a script "previous knob value" variable and use that to detect if the knob has moved before assigning a value to the current editable parameter.
Also note that any time I change the page and therefore change the meaning of the previous value, I set self._prev_k2
to None to ignore the first read on the first loop of the new page.
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.
I've just pushed a revision that uses the KnobBank
class. I'm not 100% sure I'm using it correctly; when jumping between menu levels the knob seems unresponsive for longer than it should (sometime I need to sweep it fully in both directions before it starts scrolling again). That might just be my own unfamiliarity with how it's intended to work though.
If one of you wants to pull the new version and let me know if the menu experience is better I'd appreciate it.
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.
I was suggesting using the KnobBank
class for the individual settings controlled by K2. I think that it makes less sense for the menus and K1. I agree that an encoder would be ideal here.
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.
I'm actually finding the menu navigation easier with the bank on K1; when jumping up/down frequently it's nice to reliably be on the same setting you previously were instead of randomly skipping.
I'm afraid I don't really see the benefit to adding the knob bank to K2 though; if you've pressed the button to change the setting, I'd rather have the setting be freely changeable right away, rather than needing to scroll to unlock it before I can change the value. And if you accidentally press the button you can long-press B2 to change levels & cancel the operation anyway.
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.
I'll admit that I have only run this script on my dev europi, which isn't in my rack, so I don't have the the full experience of what it is like to adjust settings in a creative context, rather than an 'explore the menu' context. So I'll rectify that.
I was imaging that when adjusting a setting, and it jumped to the current value of the knob that would be annoying in use. Especially if you don't remember the exact setting before. The cancel thing is nice, though not intuitive.
There are just not enough controls on a europi to make this kind of menu driven UI easy to use. Any decision you make will be full of compromise.
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.
Yeah, it's never going to be perfect. I think the trade-off of immediately being able to change a setting is probably more important to me than having the initial value per-setting stay consistent. I don't mind the bank on K1 though; I've been trying it out for the last couple of days and I'm getting more used to it now.
…ods & quantizers. Remove some unncessary variables, improve inline documentation, fix a couple of typos.
…e menu level. Try using the KnobBank to improve menu navigation
…e candy for a demo board or wants to test the OLED in isolation.
… since it's useless.
… the CI tests fail.
I'm likely not going to be able to do any more work on this PR in the next few days. I just wanted to thank you again for both the effort that you have put into this script as well as your patience with the review process. I think we are very close to being able to being able to merge this, and I think that it is going to be a great addition to the EuroPi. |
…entiate it. The new DIN mode means we lose the ability to start/stop the clock, but it does give us an external reset trigger instead
…s keep all the output changes better-coordinated in case one takes a long time to compute relative to the one before
I finally got a chance to install this on my EuroPi and jam a little. Very very cool script! I've got a Pam's Pro Workout and the UX feels very natural and familiar. Thanks for creating such an awesome script! I've been thinking about how to handle this PR in regards to #275. I think that we can unblock this PR by moving Euclid, Quantizer and Screensaver into This will allow us more time to decide on a long term resolution for #275 and get this PR merged and onto more folk's EuroPis. @mjaskula & @roryjamesallen does that sound like a good path forward here? |
Glad you like it! I've got Pam's New Workout and have been using it for reference, From what I understand the differences between New and Pro are pretty minor, besides the screen. Nice! I've been meaning to film a video myself showing off the Pam's script, I just don't have a good overhead camera mounting system.
So should I move the common/shared classes/functions into |
… experimental directory. Remove the stand-along Screensaver script, since it was admittedly useless.
Nice work, I'm happy with these changes. Let's make sure we get consensus from others before we merge this. |
With regards to the new lint formatter errors, I think some of the formatting you have provides more value than the suggested lint fixes, so I would suggest disabling the linter for those blocks where the unlinty format provides value with |
It looks like the biggest complaint is with the formatting of the common scales. I'd formatted it specifically with all the Is there a nice way to just tell the linter to ignore that file so I can make it readable? |
Yep, see my comment: #248 (comment) |
…set specific intervals on/off. "Easier" is relative here, as it assumes some musical background, but hopefully it'll make sense to someone.
…rily _common_ scales in the broader musical sense
Caught a couple of bugs that only kicked in when the screensaver did. Also recorded quick video showing off the module. Sorry the volume is a little low. I should have turned the gain up in OBS. |
Sounds perfect, I'm just catching up on the #275 conversation so I'll have a think but for now using experimental is a good call |
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.
The recent code changes look good to me. I haven't yet gotten a chance to play with it for real, but I'm not sure that I will be able to soon. I don't think that this is a good reason to hold anything up though. Sorry for how long this has taken.
Also, thanks for taking the time to organize the common code into the experimental dir. I like how this is shaping up.
REQUEST FOR TESTING & FEEDBACK
This program is developed, but because I'm only one person, and only have one EuroPi in my modular case, there are only so many ways I can test it.
If you have a EuroPi, please try this script out and provide feedback on the Discord server; any feature requests/bug-reports/questions/comments are welcome!
Features
Adds an alternative to the Master Clock script that lets EuroPi simulate ALM's Pamela's Workout modules.
Not every feature of Pam's is available, but it covers a lot of the bases:
din
ain
b2
Because EuroPi doesn't have an encoder, both
k1
andk2
are used to dive through the menu.k1
controls the main menu item visible, whilek2
scrolls through available options when write mode is enabled (pressb2
to toggle between write & read modes). This means once you finish turningk2
to choose an option,k1
is still in place and you won't have changed menu items. It's slightly less intuitive than ALM's encoder, but it's the best I could do with the hardware available while still keeping a dedicated start/stop button (b1
).Menu
Clock DIN modes
The clock can use external on/off signals to start/stop:
Quantization
Each channel's output can be quantized to one of a variety of scales. There is (currently) no transposition option (though by combining the output from two quantized channels you can simulate a transposition).
The available scales are:
The 1/3/5(+6|7) modes were inspired by the Doepfer A-156.
If there's a scale you enjoy using that I've missed, please submit a request and I can add it!
Development status
At time of writing, these features are implemented but not tested well and may be buggy:
ain
Known bugs & limitations
x16
to/16
(this is dictated somewhat by the PPQN limitation and the fact that the clock can only go so fast)