search

dipzza / ultrastar-song2txt

Tools that automate parts of making a song in the ultrastar txt format
GNU Affero General Public License v3.0
1 stars 0 forks source link

First implementation of cli application to estimate pitch #36

Closed dipzza closed 2 years ago

dipzza commented 2 years ago

This PR adds new code, 98.35% of which is covered by tests.

Advances in the completion of #7 by implementing a cli application that parses the UltraStar txt file of a karaoke song project, estimates pitch for each singable note from the audio file and outputs a new txt file with the estimated pitches.

The application is also organized as an independent poetry project to comply with #18. In the future the txt parser module could be split into it's own independent project if it gets enough functionality to justify it.

JJ commented 2 years ago

OTTOMH, you will probably need a few issues to clarify what challenges are there with implementing this, so that you can lay out clearly the issues and reach a decision about them. Maybe you've done that in the specific commit messages, but I'll need to see. Big PRs are not very good either for review, because they may have different obstacles for merging; atomic PRs are more helpful for that, even as issues

dipzza commented 2 years ago

Should i close this pull request then, open issues related to user stories where i discuss what should i do, and then do smaller PRs for each of the issues?

JJ commented 2 years ago

Should i close this pull request then, open issues related to user stories where i discuss what should i do, and then do smaller PRs for each of the issues?

No, why? Just make whatever changes you think are in scope, and we'll keep reviewing it until it's good enough (tm)

dipzza commented 2 years ago

Only the documentation (which is a work in progress) should be missing, apart from what you tell me to improve :)

This clearly should have been multiple PRs already, so if you prefer it the documentation could go on different PRs, or if you prefer the changes to be documented in the report to see the reasoning before merging i should have it done in a couple of days.