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

Various fixes. #205

Merged
merged 12 commits into from
Jan 20, 2024
Merged

Various fixes. #205

merged 12 commits into from
Jan 20, 2024

Conversation

sonphantrung
Copy link
Contributor

See the commits below for details.

(If only some folks stop using the word "bloat" when it's the complete opposite: more of like some mistakes were made when cleaning up the code.)

sonphantrung and others added 8 commits January 20, 2024 11:14
The `main` function is really unused, as we are only running this by importing the script & load the `Slicer` class, so just remove them.
Co-authored-by: fumiama <41315874+fumiama@users.noreply.github.com>
Co-authored-by: Ftps <63702646+Tps-F@users.noreply.github.com>
POV: You lost attention and got confused :D
@blaisewf
Copy link
Member

Hi, thanks for your PR, I'll check it out now. I understand what you say and it is clear that there are many things to clean and improve, but it is a bit complicated to do everything and testing is needed!

@blaisewf
Copy link
Member

blaisewf commented Jan 20, 2024

Okay, some points:

  • Better not to use import * and import specifically what is necessary.
  • The Unit Test will not work because the syntax is a bit different, you can check https://github.com/blaise-tk/RVC_CLI to check it.
  • Why add value="my-project"?

I'm waiting for those changes/explanations to merge your PR, again, thank you very much!

@sonphantrung
Copy link
Contributor Author

Okay, some points:

  • Better not to use import * and import specifically what is necessary.
  • The Unit Test will not work because the syntax is a bit different, you can check https://github.com/blaise-tk/RVC_CLI to check it.
  • Why add value="my-project"?

I'm waiting for those changes/explanations to merge your PR, again, thank you very much!

Ok, I fixed two of your points raised, and for the name, I add "my-project" as some folks (like me) would instantly open up the GUI and click Training (or editing the settings a bit), having to input the name over and over again seems weird. I don't really like "My-Voice" from EasyGUI, and "mi-test" looks weird for beginner folks, plus, training a voice model can be a "project" in which the user attempts to make a good-sounding model :).

@blaisewf
Copy link
Member

Perfect, thanks!

@blaisewf blaisewf merged commit 46d8aa3 into IAHispano:main Jan 20, 2024
2 checks passed
@blaisewf
Copy link
Member

If you want to add me to Discord to talk about development @blaise.tk

@sonphantrung sonphantrung deleted the various-fixes branch January 21, 2024 04:27
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.

2 participants