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

Add transposition definition #2211

Merged
merged 2 commits into from
Apr 12, 2020
Merged

Conversation

aviral243
Copy link
Member

Fixes issue mentioned in #2207 (comment)

@Ishakikani9117
Copy link
Contributor

Ishakikani9117 commented Apr 8, 2020

@aviral243 the commit by @walterbender already fixed this issue. So I think there is no need for this PR. The blocks are working perfectly. It is not throwing the transposition error anymore.

@walterbender
Copy link
Member

I am getting a bit lost regarding this issue. Which blocks are still having problems with transposition? (My earlier patch should have fixed any issues with Hertz Block.) @Ishakikani9117 Your earlier patch prevented an error from being triggered but did not properly calculate the transposition value.

@aviral243
Copy link
Member Author

@walterbender I'm not completely sure about the blocks, but during testing I found transposition to be undefined at two places inside PitchBlocks.js. One at line 2236 has been addressed by you in the latest commit. The same was addressed by @Ishakikani9117 in her PR.
Another place where transposition is undefined is line 609. Here is the console log.

Screenshot from 2020-04-08 20-19-41

This has been addressed by me in this PR.

@Ishakikani9117
Copy link
Contributor

@aviral243 Yeah you are right. But as you said it is not useful for any block.

@aviral243
Copy link
Member Author

@Ishakikani9117 I didn't say it wasn't useful for any block. I said I don't know which block.
If there's a console error triggered, some block or a combination of blocks must be using that code.

@walterbender
Copy link
Member

Can you please share a project that is triggering that error?

@aviral243
Copy link
Member Author

@walterbender Yes sure. It is actually the same project I mentioned earlier, created by you for Block API.

uber.zip

@@ -603,6 +603,11 @@ function _playPitch(args, logo, turtle, blk) {
logo.pushedNote[turtle] = true;
} else if (logo.drumStyle[turtle].length > 0) {
let drumname = last(logo.drumStyle[turtle]);
let transposition = 2 * delta;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure delta is defined either.
But I don't think we need it in this case. Just let transposition = 0 and add in the logo.transposition, it there is any.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@walterbender walterbender merged commit 8518181 into sugarlabs:master Apr 12, 2020
@aviral243 aviral243 deleted the fixTransposition branch April 12, 2020 11:08
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