Closed vale981 closed 6 years ago
Hi @vale981,
This looks like a cool new feature! I like the idea, and I'd welcome the changes in general, but as you mention there is currently a "hacky" part in that we're re-using the "setBeats()" functionality...
Do you think we should add a new item in the context menu for "Rec Beats" or similar? Or do you feel the current functionality is usable enough in its current form?
Cheers, -H
Well. It is not very intuitive, but UI savvy :).
I would add a new menu with bars not beats.
On 25 March 2018 21:23:27 CEST, Harry van Haaren notifications@github.com wrote:
Hi @vale981,
This looks like a cool new feature! I like the idea, and I'd welcome the changes in general, but as you mention there is currently a "hacky" part in that we're re-using the "setBeats()" functionality...
Do you think we should add a new item in the context menu for "Rec Beats" or similar? Or do you feel the current functionality is usable enough in its current form?
Cheers, -H
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/openAVproductions/openAV-Luppp/pull/201#issuecomment-375996104
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
P.S.: maybe we could implement one shot clips.
On 25 March 2018 21:23:27 CEST, Harry van Haaren notifications@github.com wrote:
Hi @vale981,
This looks like a cool new feature! I like the idea, and I'd welcome the changes in general, but as you mention there is currently a "hacky" part in that we're re-using the "setBeats()" functionality...
Do you think we should add a new item in the context menu for "Rec Beats" or similar? Or do you feel the current functionality is usable enough in its current form?
Cheers, -H
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/openAVproductions/openAV-Luppp/pull/201#issuecomment-375996104
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
OK cool, then wait for a few more commits that add the GUI elements to the context menu, and a new "EventType" message for LOOPER_REC_BEATS, which triggers the actual update to the looper clip class.
If you have questions or anything, just ask away here!
Re One Shot clips - sure that sounds cool too - but lets finish one PR, and get that merged. Mixing multiple features in one PR isn't a good idea :)
I opened a new issue for one shot clips so you can concentrate here on auto record stop. #204
Shall i use callbacks for the right click menu, or that strcmp magic?
I am using callbacks. BTW. All those events could be created very nicely with templates.
@vale981 when i click on custom and close the dialog without typing anything, luppp crashes. shell says: memory access error
Next time it would be much easier to use another branch than "master" for this, its kind of complicated to switch. or am maybe i am missing something. EDIT: simply pulled it into another local branch, but maybe a good idea anyway ;)
fix works for me.
Neat
I'd like to give the ClipState in the ui a pointer to the actual looperclip. So you can display how many beats a clip has.
Is that ok to do and how would you do it. Call the logic directly from jack or through an event?
I prefer this methoder over handling the setbarstorecord event to change gui, because it is now easier to get clip info in the gui.
But! It may not fit into the design structure.
Hey @vale981, interesting approach... but not a good design choice in the Luppp context.
The core of Luppp is entirely split - the DSP code, and the UI code never access the same data. This allows (or enforces I suppose) the code to be lock-free, and real-time safe all the time.
What would happen if we de-allocate or re-allocate an instance? The pointer will be invalid - possibly for a short amount of time, but also possibly could cause a segfault. I'm aware that such a strong split between UI and DSP might seem overkill, but it does scale well.
The message passing approach with the Event struct that Luppp uses throughout enables transfer of information between the two halves.
As such, I think the best approach is to add an Event struct type, and use that to signal the info back to the UI.
Hope this makes sense - any questions just let me know! -Harry
It does! I'll look into it.
I clearly see the advantage of this event driven approach for the means of realtime safety in the DSP code. Certainly this code has some rough edges. But it is the first step in the right direction (so i hope).
By the way. If a sample is loaded, the name is set directly (no gui event).
Cheers.
Yes! I did change the indentation format on my main machine, but forgot to do so on my laptop. It displays tabs and [4 Spaces] as exactly the same indent-level, so I didn't realize it.
Gonna fix that up.
We can run clang-format over it. But that would be another pull request.
This PR would fix #101.
Re clang-format: yes great idea. I'd love to have a consistent method to keep the source clean. Do you have experience with setting up clang-format? A git commit hook to automatically fix it would be awesome :)
Yes noted that #101 will close on merging of this. Do you have any outstanding work todo on this PR, or it is ready for merge in your opinion?
Cheers, -H
I deem it ready.
But maybe sth. Comes to my mind.
@vale981, I'll have a review now and if it looks good clicky on the merge button :)
[OT] Side note, I don't know if you use # IRC, but there's an #openav channel on irc.freenode.net where I hang out if i'm coding OpenAV stuff :)
So here we have it. Now it shows beats/bars.
I tested it. In general it works fine, its a great feature. Thank you for this!
Just some ideas from me, but i would be also happy it this could be merged as it is.
@vale981 will there be any further progress or do you think its ready? i could mark this as please merge, if you are done.
Sorry I was away for a bit. I'll check in soon.
On 5 April 2018 10:29:42 CEST, Georg Krause notifications@github.com wrote:
@vale981 will there be any further progress or do you think its ready? i could mark this as please merge, if you are done.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/openAVproductions/openAV-Luppp/pull/201#issuecomment-378859497
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
dont feel pressure, just wanted to ask ;)
Default clip length would be inflexible. There is no good way of coping with this in the current single bar mode.
Having a Giada like sequencer would be a treat.
there might be an missunderstanding. i dont want to fix the length, just change the default length. currently its set to endless, which in fact isnt really length, but an setting for the record length. it would be nice to define a default, eg 8. you can manually stop the recording before anyway, right? So its like an maximum record length for the hole session.
Yes but one may want to record longer.... I would feel very uncomfortable if i could jam along indefinitely.
on smaller displays the text gets chopped off / overlaps with the title.
Yes but one may want to record longer.... I would feel very uncomfortable if i could jam along indefinitely.
I dont want to prevent this, you could always change it to endless for each clip again. i just think it would be nice to be able to change the state which all clips have at the beginning.
on smaller displays the text gets chopped off / overlaps with the title.
Currently if you make the window smaller the text jumps out of the clip. Two ideas:
Maybe we should move this display-thing to another commit since we need to think about again and maybe also add an setting to make it optionally. This way we could merge this one already.
Fixed it by trimming the text.
So. It is now color coded and the text trim is senistive to the beats/bars indicator.
So, I think it is good to merge :).
did a fix test, looks better but maybe we find a way to put the center of the numbers exactly over each other. and only the first track scene one is colored, others are white.
Sorry for complaining again and again, if you dont want to fix it i could do it in some days.
Thank you very much for the work, i really like the feature!
They are colored if there are 'beatsToRecord'. This is the same on all tracks.
The numbers are aligned to the right. Center alignment is harder and doesn't do much good.
ah, thanks for explaining the coloring. maybe choosing a color which is already part of luppp? maybe the red or blue from the buttons?
eg 1 over 4 simply looks sloping.
Anyway, since you are the author, if you tell its ready, i mark it as please merge and we will see what Harry say. Again: thanks a lot.
Got ya! Fixed that. There is another thingy that I am trying to get at though.
(Endless is always marked...)
Yes, Choosing another color would be better!
@vale981 - please look into your editor white-space settings. There are mixed tabs/spaces and trailing whitespaces in the current code.
In general, most of the code itself is fine - I'll post a few review comments now.
Please do not merge. There is a bug that i am trying to iron out.
Here we go! All good now.
Something actually undid the major feature ^^.
I can squash some commits if you want it more compact.
Something actually undid the major feature ^^.
@vale981 what do you mean?
This little hack uses the beats menu in the context menu to set the maximum beats to record (Rounded up to bars).
If no beat number is set, record works as usual. The beat number is reset upon track reset. Cheers.