Skip to content
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

adding Sample Folder #7538

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

adding Sample Folder #7538

wants to merge 59 commits into from

Conversation

szeli1
Copy link
Contributor

@szeli1 szeli1 commented Oct 9, 2024

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. SampleFolder class 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, SampleFolder will 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. SampleFolder will scan for ".flac" samples inside ProjectFolder, ProjectFolder/Used and ProjectFolder/Unused and will load them if something is requesting a shared_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 named LmmsExporterSample, this name isn't the best, so any better suggestion is appreciated! This class exports any SampleBuffer in a new thread as a ".flac" file.

How to test:

  1. Open a new project and start saving it. You should see an option called "Save project inside sample folder" in the save dialog. Make sure that option is on when saving. Once saved, a folder should appear in the selected location that contains the project file, and 2 other folders called "Used" and "Unused". Load a sample inside a sample clip. Open the context menu and click the new "save to sample folder" option. This should reexport the loaded sample to the "Used" folder. Save the project and reload lmms and reopen the project. The sample should be loaded as if nothing happened (to how the samples are handled). Delete the sample clip and save the project. The exported sample should have moved to the "Unused" folder.
  2. Open a new project. Load a sample inside a sample clip and open the context menu and click the "save to sample folder" option. The sample should be reexported to a new folder inside lmms/ (where the plugins folder 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

@szeli1 szeli1 marked this pull request as draft October 9, 2024 21:37
@szeli1
Copy link
Contributor Author

szeli1 commented Oct 26, 2024

I'm no longer working on this feature.

@szeli1 szeli1 closed this Oct 26, 2024
@szeli1 szeli1 reopened this Dec 8, 2024
@sakertooth
Copy link
Contributor

This pull request aims to add a way and a place to store sample files for projects.

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?

@szeli1
Copy link
Contributor Author

szeli1 commented Dec 17, 2024

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:

  1. Option to store samples locally near the project file
  2. Option to store samples globally inside a designated path
  3. Better sample management (keeping track of all used and unused samples)
  4. Utility (unique name generation and keeping a list of available samples)
  5. Ability to rewrite this class to use UUIDs
  6. Ability for this class to manage all file name related tasks (so other classes don't have to store paths to samples in the future)

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!

@sakertooth
Copy link
Contributor

I wouldn't say that I'm convinced or if this is the best way to do this.

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.

This PR has secondary goals that include:

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 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 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 tried to follow most of these guidelines and I tried to implement most of these features.

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.

@szeli1
Copy link
Contributor Author

szeli1 commented Dec 17, 2024

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.

Please ignore my feelings about this PR, it will lead nowhere

A good practice for PRs is to keep them small and focused.

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.

PR is still trying to do many things all at once

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.

it seems as if you have coupled many somewhat related things, and as a consequence the PR lacks direction

What direction should this PR aim for? I aim to completely implement and finish the folder component of "Sample folder" and exporting outside of AudioEngine. The UUID system will replace about half of the sample related functions inside SampleFolder, so I aim to enable this change to be as simple as possible.

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 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

you are biting more than you can chew

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)?

@sakertooth
Copy link
Contributor

Please ignore my feelings about this PR, it will lead nowhere

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.

@szeli1
Copy link
Contributor Author

szeli1 commented Dec 17, 2024

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).

@sakertooth
Copy link
Contributor

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

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.

@sakertooth
Copy link
Contributor

I don't feel certain.

But you mentioned you were not convinced that your work was appropriate for the problems you were trying to solve?

@szeli1
Copy link
Contributor Author

szeli1 commented Dec 17, 2024

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.

@sakertooth
Copy link
Contributor

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.

@szeli1
Copy link
Contributor Author

szeli1 commented Dec 17, 2024

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.

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.

@szeli1 szeli1 marked this pull request as ready for review December 19, 2024 20:00
@michaelgregorius
Copy link
Contributor

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

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.

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.

@michaelgregorius
Copy link
Contributor

I have created the following discussion to not derail this PR: #7642.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants