adding Sample Folder #7538
adding Sample Folder #7538
Conversation
|
I'm no longer working on this feature. |
This has been brought up many times, but why are you convinced that hundreds of lines of code are necessary to add a way and a place to store sample files for projects? |
I wouldn't say that I'm convinced or if this is the best way to do this. This PR has secondary goals that include:
I believe these goals have been agreed to in a discussion about this feature by multiple users. There was an other discussion about this feature also. I believe you didn't object to these goals when those discussions happened. I tried to follow most of these guidelines and I tried to implement most of these features. Some features have been left out (like the ability to export samples that are still being recorded, so in case of a crash the sample isn't lost) I hope this comment clarifies the larger than expected code amount. If not, feel free to ask more questions! |
If you are not convinced that your work is necessary or a satisfactory solution to a problem, then this shows uncertainty. I applaud your effort, but if you are uncertain about it then I am too.
A good practice for PRs is to keep them small and focused. It helps with reviewing them, minimizes regressions, and allows the team to have a better grasp of the PR and be more confident with knowing how and why the changes were made. However, it seems as if you have coupled many somewhat related things, and as a consequence the PR lacks direction. At the end of the day, the PR is still trying to do many things all at once. Stick to doing one thing please :)
I have read the goals you have listed, and I get the feeling the goals we mentioned in our discussions are different than what you had in mind. This isn't your fault really, we definitely need to get better at specifying what exactly we want to do for the project and what our overall direction is. We did this very nicely with working towards the 1.3 release, but a lot of the secondary goals, especially relating to sample functionality and sample workflow, could be better explained clearly somewhere, and not just in discussions floating on Discord. This "specifying of goals" is a team effort: no one developer can make all the decisions here. As small as the team size is, working on a project like this and making goals for it requires everyone to be considered. The community and developers alike.
I understand, but from my perspective, you are biting more than you can chew. I was like this once: I wanted to fix everything, rewrite the whole project, start from scratch. I still feel that way at times. However, I realized that it probably isn't as simple as that. A better, safer alternative is to take it one step at a time. |
Please ignore my feelings about this PR, it will lead nowhere
My issue is that I can't build on unmerged features. My FFT filter effect is still on hold because it is waiting for #7158. Other developers have advised me to merge features into 1 pull request rather than to make multiple smaller ones because it will be approved faster that way. I believe only 1 of my features were merged after 1 year of work.
I would rather not change this with this PR, but I will keep this in mind in the future. I think this PR accomplishes the goals I believe were set.
What direction should this PR aim for? I aim to completely implement and finish the folder component of "Sample folder" and exporting outside of
I agree with this. I usually feel unguided regarding the development direction. I usually try to communicate my ideas and sometimes nobody responds (this happened to the 1. discussion about this feature, where completely different goals were set). Edit: maybe I should take part in other discussions related to this project, so I can be a better contributor and take part in this team effort
What do you mean by this in the scope of this PR? Could you explain it with some examples? What are you afraid of (or what feeling leads you to this conclusion)? |
Sorry, but I refuse your request. You feel uncertain, and as such, I am uncertain. As a result of this, the conversation will amount to nothing, and not because the feelings about the PR are being ignored. |
How should one feel about a pull request? I don't feel certain. I don't feel uncertain. This is why I would rather avoid talking about this. I would like to focus on the facts, that sample track recording stage 1 needs audio exporting, an other fact is the code that accomplishes this (exists). |
That's normal, you're not doing anything wrong. Partaking in the discussions is fine, but even for people that aren't in the Discord but still contribute here (michaelgregorius comes to mind), we still need to improve how we specify our goals for the project. |
But you mentioned you were not convinced that your work was appropriate for the problems you were trying to solve? |
Yes, because this PR will change as reviews are done. I said this: "I wouldn't say that I'm convinced or if this is the best way to do this." I think if more experienced developers review this pull request, they might find issues that need to be fixed. |
That's fair. However, my opinion is that the PR just doesn't seem really focused. It accomplishes too many things all at once and it is really hard to understand if the PR has validity (validity, meaning if the PR is logically sound in its reasoning and consists of changes we need). Put simply, I do not understand what the PR does exactly. There's not a single, clear defined goal that I can gather unfortunately, but many slightly vague goals which lack direction and clarity in my opinion. I also mentioned before that I personally think the problem you are trying to solve, "to add a way and a place to store sample files for projects." has a far simpler solution. The way we create a place for users to store sample files is by giving them an option to specify a path to put sample files in. That's it. I don't really want to go into anything about this, because I really do think it is, in its simplest form, that rudimentary. Choosing just one of those things to start off would be a good start, even though there are maybe other developers possibly working on that same problem. However, having just one singular idea makes it so much easier to understand the PR and your vision as the author. |
You are right, it is simpler for that specific problem. I'm starting to understand your point about making more specific and clear Pull Requests. I guess I will make a new PR that includes the exporter and what you've described above. Thanks for taking the time to get to this point with one of the contributors. I still don't know what will happen to this PR once an other one exists with the same export code, I guess then I will rebase this branch. |
As development is taking place here on GitHub to me it would be a logical step to also make the goals of the project transparent on GitHub so that people can contribute in a meaningful way even if they are not on the Discord. My personal impression is that the project is lacking leadership, e.g. someone like Ton Roosendaal for Blender or Guido van Rossum for Python. However, it might only be my impression because I am not on Discord and therefore things are not transparent to me. My personal goal is to help push LMMS more towards a "professional" direction and to make it more on par with other DAWs (see for example my PR for faders with a dB scale). If you compare LMMS to other DAWs you will find that it is lacking quite a lot. However, my time is sparse so unfortunately I cannot help with the real big tasks that would be necessary to do so (real-time safety, audio graph with latency compensation, more sophisticated features for audio drivers, e.g. multiple inputs and outputs, etc.). That's also the reason why I do not want to get too involved, e.g. via Discord. I guess it might make sense to discuss this in a discussion thread which I might create. |
|
I have created the following discussion to not derail this PR: #7642. |
messmerd
left a comment
messmerd
left a comment
There was a problem hiding this comment.
Not a thorough review, but just a few things that stood out
| //! outputLocationAndName: should include path and name, could include ".flac" | ||
| void startExporting(const QString& outputLocationAndName, std::shared_ptr<const SampleBuffer> buffer); |
There was a problem hiding this comment.
Given how you use it, const std::filesystem::path& might be a better option for the type. And the name "outputLocationAndName" is very clunky, so you could shorten it to "output" or something. If it's a std::filesystem::path type, it'll be obvious that "output" is a file output path.
| { | ||
| stopExporting(); | ||
| m_isThreadRunning = true; | ||
| m_thread = new std::thread(&LmmsExporterSample::threadedExportFunction, this, &m_abortExport); |
There was a problem hiding this comment.
If you just need a null state, why not use std::optional<std::thread>?
There was a problem hiding this comment.
I need a new thread. The thread is used until every job is finished, then it is joined.
| } | ||
|
|
||
|
|
||
| void LmmsExporterSample::threadedExportFunction(LmmsExporterSample* thisExporter, volatile bool* abortExport) |
There was a problem hiding this comment.
Using volatile for cancellation makes me feel a bit uneasy. Maybe @sakertooth could weigh in here, since he's fairly knowledgeable about multi-threading.
There was a problem hiding this comment.
I basically never use volatile, and we could probably just use std::atomic_flag or even std::atomic<bool>.
Other thoughts:
- We should be using the
ThreadPoolclass here to submit the export as a task to one of the threads. - I believe you could just make this a member function, enqueued into the
ThreadPoolwith a lambda threadExportFunctionis kind of an odd name,runExportor something along those lines would be better and simpler IMO.
|
|
||
| private: | ||
| Sample m_sample; | ||
| QString m_sampleFile; |
There was a problem hiding this comment.
This seems redundant since m_sample should already store the file path.
|
|
||
| class Engine; | ||
|
|
||
| const QString COMMON_SAMPLE_FOLDER = "lmms_recorded_samples/"; |
There was a problem hiding this comment.
Why "lmms_recorded_samples"? Isn't this meant to be in the "LMMS" folder alongside "projects/", "presets/", etc?
There was a problem hiding this comment.
Agreeing, LMMS is clear from the parent directory. Also, most Linux systems seem to prefer minuses over underscores:
$ ls -la /usr/share | grep -c _
9
$ ls -la /usr/share | grep -c -
340
| thisExporter->m_isThreadRunning = false; | ||
| } | ||
|
|
||
| bool LmmsExporterSample::openFile(const QString& outputLocationAndName, std::shared_ptr<const SampleBuffer> buffer) |
There was a problem hiding this comment.
It seems like you're trying to reinvent the wheel here. SampleDecoder should be capable of reading .flac files.
There was a problem hiding this comment.
It is opening a file to export to.
|
After discussing this on Discord I found it best to gather all my thoughts here. I agree with @sakertooth that this PR lacks a clear direction. Let's zoom out and talk about the problem this PR is actually trying to solve: we need a place to save recordings. 1. Store samples locally near the project fileI'm a fan of this idea. We already have a feature called "project bundle", that can be found in the save as dialog. I think that feature should be extended to handle recordings too. There is no reason to write a whole separate system for that. 2. Where do we place recordings if the user doesn't care?You opted to place them in a global sample folder. I think this is a really, really bad idea. Scenario: a user has been using the recording feature for some months and now there's 300+ samples in the global sample directory. This has become unbearable to the user and they want to organize it a bit better. But there's no way to do that now without breaking all projects. Solution: just store everything close to the project by default, and there is no risk of clutter even if the user doesn't care how they save the project. 3. What if the user WANTED to share a sample between projects?Easy. Let the user save it in the 4. How do we keep track of which samples are used and not?If a sample is stored together with the project, it's because it's used by that project. No need to track it. If a sample is in the 5. But what will happen to unused samples?IMO if a sample is saved (by LMMS) to a project-local directory, and the clip is deleted from the project, it should be deleted from the file system too. Like @JohannesLorenz pointed out on Discord we don't keep deleted melody patterns or deleted instruments, why should we keep deleted samples? 6. "Deleting sound files off of users computers is a bad idea"Scenario: a user records a sample in project A, then creates a new project B and opens the sample file from a project A's sample directory, then goes back to project A and deletes the sample clip... If LMMS deletes sound files from the filesystem it would break project B! Solution: If deleting sound files is a bad idea, then don't use sound files. I mean save them as FLAC, but call them something like I admit that point 5 and 6 is more related to the recording PR than this, but I think they are relevant to the problems this PR is trying to solve. You have put down a lot of good work here @szeli1 and I don't mean to ruin that. Even if you are eager to merge this, you are probably more eager to get going with the recording, and if we can get there using a simpler solution and the systems we already have in place, it will probably get done faster. |
I don't want to merge this in the near future. It was voted to use #7627 for the time being.
Your argument builds upon this idea, to not use global sample folder.
If we are going with your idea, then this makes sense. If global sample folder is implemented along with project folders, better to use the same code for both if possible.
Save the project as a bundle / project folder. Name recordings
I tested project bundles and deleted sounds were not removed from a bundled project's folder. Newly imported sounds also weren't added to the project bundle. I see why would you like to use project bundle, but improvements are ought to be made to use it as a project folder. Thinking about it, I may be okay with the user explicitly telling lmms to delete the currently unused files. I still think the files that leave lmms's project file must be managed by the user. Using extensions like I appreciate your comment @allejok96 even if it doesn't seem like it. |
Fair enough. If we are going for a global sample folder, I agree we must have a management system. I still dislike it, but there is no point in discussing that since the idea is not yours. We need to find the people who wanted this and ask what their motivations were. Maybe they just want samples to be saved without having to select a location. Or maybe they want to easily share all samples from one project to another. There are simpler solutions for both cases. Just because they suggested a global sample folder doesn't mean they understand the implications or thought of alternative solutions. |
|
IMO a management system would be needed anyway. First of all, the DAWs that I know (Reaper, Bitwig) provide functionality to save a project in such a way so that all samples that are referenced by the project and that come from outside of the project are saved close to the project. In Bitwig this is a menu option which is called something like "Collect and save..." (in German it's "Sammeln und sichern..."). Every time that LMMS deletes a sample this should be a result of an explicit user interaction. This might be implemented with a dialog which lists the samples which are used/referenced by the project. User can then for example select entries and delete them. It would also be helpful if such a dialog would indicated samples as "project samples", i.e. samples that are saved close to the project, and "external samples". For external samples it is highly likely that they come from sample collections that users have collected over time. LMMS must not delete any samples from these locations without explicit user interactions. Imagine building such a library and using some drum samples that you like in a project. You then delete the sample clip from the project and only realize later in another project that LMMS has deleted it from you personal collection. I am pretty sure users would quickly stop using LMMS if it behaved like this. So do not delete any files without explicit user interactions! Also do not delete sample files if a sample clip is deleted, even if it is stored close to the project. Users might want to go through the following scenario:
I would not be happy if I recorded a sample clip only to find that it was delete because I removed a clip which should simply be a reference to the sample like in most other DAW software. Most DAWs treat MIDI and samples differently, so the argument "we don't keep deleted melody patterns or deleted instruments, why should we keep deleted samples" does not work for me. Also please do not invent things like It's always helpful to check how other DAWs solve these problems before inventing own things which might work worse than in the other applications. |
|
Thanks for the good insight in how other DAWs handle it @michaelgregorius ! So if you record a sound in reaper, save the project, delete the sound and re-save the project, the sound is still saved somewhere?
This is only a problem as soon as we save the recording in a format and location that the user can easily access. From that point we can never delete the file. Or even suggest it is unused. Because no management system can know what the user does when LMMS is closed. Where does Thunderbird save the email attachments when you receive an email? Idk. Somewhere in the application folder. If I want to use the attachment in another program I right click and save it. No one complains about that being a restriction. But if I delete the email I assume the attachment isn't left wasting space on my hard drive. Why should recorded sounds within a project be so different? Sorry, I didn't want to start the whole deletion discussion on this PR... Please redirect to the appropriate place. |
Yes, that correct. There might be different possible configurations in REAPER. IIRC the default is to have one global folder where recorded sound files are stored. REAPER has a dialog that shows the content of that location and it can be used to manage/delete these files. What's important to understand is that audio clips/regions are only references to files. To a certain degree they are not representations of the files themselves. So if I drag and drop the same file three times to different tracks in REAPER then it will create three audio clips/regions which will all reference the same file. If I change the content of that file outside of REAPER then the content of these regions will change as well. If I delete one of these clips then the file will still exist and the other two clips can still reference it. So the users do not delete files and their data but clips which only reference the files.
That's correct. But why shouldn't user be allowed to easily access their files? Perhaps they want to edit one or more files with a audio editor. Why should this be made hard for them? The fact that no one knows what the user wants to do when LMMS is closed is also a good argument to not delete files without explicit user interactions. All that LMMS can do is provide some good management tools which in part make use of LMMS internal knowledge. LMMS can for example know all audio files that are referenced in a project. It can know if they are saved in the project folder (or any other folder managed by LMMS) or if they are saved in external folders.
One thing to notice is that Thunderbird treats your mails and attachments very responsibly. I do not use Thunderbird but I assume that it also uses the concept of a trash can into which deleted items are moved before they can be deleted for good. Every final deletion, be it the full mail or only the attachment, is a conscious user interaction. If you bypass the thrash can you will likely need to use a special key combination which again is a conscious decision by the users. Last but not least email software and audio software solve different problems so they cannot be easily compared.
I guess it should be a discussion in the discussion area. |
The user is the management system. Display a dialog that lists unused samples (in the project folder) and the user can decide if they would like to keep or delete them. It would be nice if deleted samples would be sent to the "trash" and not immediately deleted. |
|
After asking szeli that they are OK with continuing thread abuse to talk about sample deletion ... 😰
The use case you mention here could simply be solved by letting LMMS parse all LMMS projects in the project folder and check if any of them references that sample. Which I would indeed prefer over any kind of management system. However, even if no project references the sample, it might sound a bit dangerous to delete it - the user may still want to keep it for later use. So I would rather say, let us implement a dialogue which can be opened from the settings, which lists all unused samples (not used by any of the LMMS projects) and allows you to delete them. That being said, I see no reason to store the samples "close to the project" - This should IMO exactly be done if the user saves the project "with resources".
We could do that, though I don't see much advantage, given my approach above. 1 Folder is easier to handle than 2 folders. Otherwise, users always need to check 2 folders when searching for a specific sample. |
IMO this opens just another can of worms. Some thoughts:
Therefore I'd propose to implement the dialog for the currently opened dialog as a first step and then to see if it can be improved. I guess that might already provide some challenges:
What does "with resources" mean? That they are stored as individual files "close to the project"? That they are embedded in the project as base64 or similar. For the recording feature it would be best if the samples were stored as individual files on disk so that users can edit them with external editors.
The trash is simply an additional security mechanism which the operating system provides. I do not intend it to be another folder where users store their samples and search them all the time. 😅 I am not sure how easy it is to implement with Qt though. |
|
The problems with this discussion
Let's instead identify features that an end user might want, and then how they might be designed. Features people want
Right click > Add to library. This would save the sample in the global sample dir for easy access and sharing.
Right click > Open in another program. This is a common feature. Where the file is saved is irrelevant for this feature.
If a sample is recorded and then deleted AND if the user does not intend to use it in any other project or software, it should be deleted from disk to not waste disk space. The user shouldn't need to regularly check for and delete unused samples, like we are back in the days of CCleaner.
Don't require the user to enter a file name for each sample when saving the project. Question relevant to this PRDo we need to make all recorded samples easily accessible by default? I propose we have a hidden sample storage and a public sample storage.
Lastly...
I thought it might be nice because it makes it easy to track which projects use the most disk space. But there might be other strong reasons why that is a bad idea. Forgive me for this wall of text. I hope by now I managed to communicate my thoughts. I will not make further comments. |
They didn't really say that they wanted this. They wanted global sample folders.
It is true that it is irrelevant where the file is saved, but it should be a
Yes, you can't really detect if an other software uses the file any you can't detect the users intentions I think, so better let the user manage their files. Maybe provide some help.
Agree. I would like to propose this solution: Currently the PR supports both global and local sample folders (= project folders). Keep it this way. Store saved audio with normal ".flac" extensions. Name new samples with project name + number. Add a dialog window that shows the unused samples either in the project folder or in the global sample folder. Add some buttons to delete some or all unused audio. |
6 participants
closes #7452
This pull request aims to add a way and a place to store sample files for projects.
A new class is added called
SampleFolder. It will store audio files in 2 ways: relative (Project Folder) and absolute (inside a folder that can store all audio files for multiple projects).All sample loading and saving should be routed through
SampleFolder.SampleFolderclass will require 3 new folders for each project. A project folder and a "Used" and a "Unused" folder placed inside it. There will be an option to save the project inside a project folder in the main window menus. If the project file is inside a folder with the same name,SampleFolderwill interpret it as a project folder. If this condition isn't true, a new global "everything" folder will be used as the current project folder.SampleFolderwill scan for ".flac" samples insideProjectFolder,ProjectFolder/UsedandProjectFolder/Unusedand will load them if something is requesting ashared_ptr<SampleBuffer>. The "Used" and the "Unused" folder makes managing files for the users easy, If they wants to delete the unused files, they easily can.A new class is added for exporting samples outside of the
AudioEngine. This class is namedLmmsExporterSample, this name isn't the best, so any better suggestion is appreciated! This class exports anySampleBufferin a new thread as a ".flac" file.How to test:
lmms/(where thepluginsfolder is), this folder is called "lmms_recorded_samples" (should be renamed in my opinion). Save the project without the sample folder option and reload the project. The sample should be loaded in correctly.this allows #5990 to be finished