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

adjusted modify_rotate to preserve nonlinear scale data #158

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

vsicurella
Copy link
Collaborator

Fix for #140

Instead of transposing in-place, we start transposing on the line after the new root through the equave. The transposed equave becomes the degree by which you can use to invert the rotation. Then we cycle back to the beginning of the scale and transpose up to the new root. Then we push the original equave to the end of the scale.

Copy link
Collaborator

@meszaros-lajos-gyorgy meszaros-lajos-gyorgy left a comment

Choose a reason for hiding this comment

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

Please check if those console.logs are needed and if not then please remove them to minimize accidental console spamming.
Apart from that I like the renaming of variables and the reorganizing of code, the code seems more readable to me.

let deg = (i + degree) % lines.length
let index = i - 1
rotatedLines[index] = moduloLine(transposeLine(lines[deg], transposer), equave)
console.log(index)
console.log(degree)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would recommend cleaning up console.log calls once the development of a feature have been finished.

@@ -275,23 +275,24 @@ function modify_rotate() {
console.log(transposer)
console.log(equave)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the PR isn't about these lines, but if possible and no longer needed, then please remove these console logs

@vsicurella
Copy link
Collaborator Author

no worries, they were in there previously, but since the sorting issue is resolved I don't think they're needed anymore.

Copy link
Collaborator

@meszaros-lajos-gyorgy meszaros-lajos-gyorgy left a comment

Choose a reason for hiding this comment

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

Nicely done!

@meszaros-lajos-gyorgy meszaros-lajos-gyorgy merged commit 8a6ccbd into SeanArchibald:develop Feb 16, 2022
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