openAVproductions / openAV-Luppp

Luppp is a live performance tool, created by OpenAV productions.
http://openavproductions.com/luppp
GNU General Public License v3.0
258 stars 45 forks source link

set track and scene number on startup #145

Closed georgkrause closed 6 years ago

georgkrause commented 7 years ago

This patch allows the user to start Luppp with specified number of tracks and scenes by running

luppp -scenes x tracks y

To save the user from problems with ram and cpu i set the limit for both to 20.

Warning: Don't merge yet! There are two problems i didn't solved, which i maybe cannot solve.

When i have time i can do some more research, but i don't think i can handle this in the next time (because of a lack of skill ;), but maybe somebody can take a look and solve the issues.

harryhaaren commented 7 years ago

I don't like the CLI arguments - users don't launch Luppp from the command line, nor should they have to. I'm OK with merging this functionality (for testing purposes, and power users) but we need to be able to set the Tracks/Scenes using the Options / Setup dialog or something. Just a note - I'll do a better review when you ping me that you're happy with the code.

georgkrause commented 7 years ago

well, i totally agree with that. but: if this CLI stuff works, it should be pretty easy to set the tracks via config file and in the next step, integrate a settings dialog which edits the config file. but more important: at the moment its not working as expected and at the moment i am unable to checkout how to fix ;)

harryhaaren commented 7 years ago

I checked out the code, builds fine. You say its not working as expected - but didn't describe exactly what the issues are. I guess you mean resizing of the window, and scaling of the widgets in the tracks.

Eg, run with 2 tracks, 2 scenes, and the widgets sizes' are all mixed up because there are some hard-coded constants in the UI initializeation that are currently set for 8 tracks, and don't scale properly with a different number of tracks/scenes. Correct?

georgkrause commented 7 years ago

well, the gui isnt an issue here since i am working on this in another branch and pr.

the topic here is to run the engine with defined number of tracks or scenes. my aim here is to get help and reviews ;)

the problems are:

harryhaaren commented 7 years ago

Aha, sorry for the misunderstanding - my initial quick look noticed GUI scaling issues - and I didn't test audio at all for this PR yet :)

Re: Number of buffers I think we will have to check the DSP classes, and check for hard coded constants. A good method is to do a search for the number "8" or else try "8;", (think about for(int i = 0; i < 8; i++) ;) This should lead us to any instances of the hard-coded number 8 (for tracks) or 10 (for scenes), and we can investigate a fix then. I presume somewhere just wasn't using the NTRACKS define, which is now a variable IIRC. Is this what you mean by the first problem bullet point?

Re: segfault Also, re more than 11 scenes instead of more than 10, I guess that accessing tracks[10] is actually a memory-access violation, but due to mem layout its not showing up there yet. Yes these are bad (and difficult to debug) bugs - but given we can reproduce them by changing a constant, we know how to make them appear/dissappear, which is a powerful method to finding the root cause :)

Hope that helps a bit, if not, please detail the two bullet-points in more detail and I can try help identify fixes for them. Cheers, -H

georgkrause commented 7 years ago

the last commit fixed the segfault. the next step is to solve the buffer problems.

georgkrause commented 7 years ago

in fact i tried this before, but this is not working because nscenes isnt a constant:

array bound is not an integer constant before »]« token LooperClip* clips[nscenes];

My solution works and there is no warning.

georgkrause commented 7 years ago

New problem: Loading session files (further debugging needed)

georgkrause commented 7 years ago

playing a loaded clip with number of scenes other than 10 also leads to a crash

harryhaaren commented 7 years ago

Sorry, but allocating memory in a header file is wrong, and won't be accepted. There are other solutions (look into std::vector<>:: push_back() to dynamically grow arrays). Allocating in the header isn't a proper solution.

Next steps may be to store looper clips in a vector to resize as needed:

header:
std::vector<LooperClip*> looperClips;

impl:
for(int i = 0; ... )
    looperClips.push_back(new LooperClip() );
georgkrause commented 7 years ago

@harryhaaren thanks for the feedback. using the vectors seems to be a good idea. sorry, i am that focused on understanding how the hole think works that i didnt thought about other solutions. i will try to implement the vectory and lets see what happens ;)

georgkrause commented 6 years ago

I want to work again on this hole topic, but since the commit history is dirty, i will start from the current master again and try to keep it clean. So i am closing this but try to apply all the tips i got here ;)