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

Support Guitar Pro 7 track volume #445

Merged

Conversation

jordanske
Copy link
Contributor

Added support for importing the track volumes from Guitar Pro 7 file format.

@Danielku15
Copy link
Member

Can you please first open an issue describing in detail what is the missing feature. It is not really comfortable to just get some code provided without deeper insight what you're trying to solve.

@jordanske
Copy link
Contributor Author

Sorry, this is the first time I contribute something on Github.
I have created a new issue with an explanation. #446

Also, am I correct to assume that the web check error is due to the test expecting a different volume than set in Guitar Pro? Which gets imported correctly now from the grace-beats.gp.

@Danielku15
Copy link
Member

@jordanske That's fine, thanks for creating the issue describing the bug you're solving. 😉 The failing tests rather indicate that some test expectations need also to be adapted. The old code will "wrongly" keep the default volume which is checked in MidiFileGenerator.test.ts Line 201 and 210. With your change we now read different values from the file and the expected events need update.

Copy link
Member

@Danielku15 Danielku15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good in general. I added one small remark on simplifying the parsing of the channel strip parameters. Could you ensure (like described above) that the tests are updated with the right new values? Also it would be great if you can add some dedicated test to this new feature. e.g. One new testfile with multiple tracks having different volumes, and then a new test checking the expected volumes.

src/importer/GpifParser.ts Outdated Show resolved Hide resolved
@jordanske
Copy link
Contributor Author

I simplified the parser code, and fixed the Grace Beats test.
I also made a new .gp file with multiple track and a new test that checks if the values are correctly imported.

Should I make a new pull request (and a new issue) for importing balance values, or can I implement it in this one?

@Danielku15
Copy link
Member

I would recommend adding the balance information directly as part of this PR. I guess it is just one of these parameters in the list so it should be fairly straight forward without big impact to add it here.

@jordanske
Copy link
Contributor Author

I added the balance and also made a new test for it.

@Danielku15 Danielku15 self-assigned this Dec 2, 2020
@Danielku15 Danielku15 merged commit 72512ce into CoderLine:develop Dec 2, 2020
@Danielku15
Copy link
Member

Thanks a lot for this contribution 😃

@jordanske
Copy link
Contributor Author

Awesome!
You're welcome and thanks!

@jordanske jordanske deleted the feature/guitarpro7-track-volume branch December 2, 2020 20:01
This was referenced Jan 17, 2021
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