-
Notifications
You must be signed in to change notification settings - Fork 662
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
Voice cross stave support in System #1434
Conversation
Ready for review |
Thanks Rodrigo. This is good start. Re: the first two examples, are they intentionally off? |
I wouldn't say 'intentionally' :) But this is the state of mixed-direction beam currently. Vex flow has always allowed mixed direction on a single staff, but there are no test cases for it. It seems logical to include them with the cross-beam stuff. Additional bugs/PRs can be opened against the those issues. I would expect more than one change will be required. |
@0xfe Rodrigo mentioned in an earlier PR that these stem mismatch issues are outside the scope of the PR, they already existed before and will be handled separately. |
This is right. The aim of this PR is simply to allow System to work with a Voice whose Notes are located on different Staves. The tests are there to show the issues related with the representation of these Voices. These issues will have to be solved in future PRs. |
Let us progress with this one this weekend 💪 and make a new release 🥳 ! |
I decided to make a minimal change to Beam based on the work done by @Silverwolf90 in #97 to get a better representation of the cross beams. I hope that this improves the acceptance of this PR :) |
Ready for review :) |
@AaronDavidNewman I believe that you have tested already the system.ts related changes of this PR. Any issues? Did you realise that I also changed beam.ts fixing the stem related rendering problems? See outputs above. |
no I haven't been paying attention. I'll try to add some of the cases from the other issue, if that's what you need. Def. looks better with the beaming change. I haven't spent any additional time with system.ts but I haven't been able to find a better way to do it with a single structure. We need to keep track of which notes are on which stave somehow. |
Hi Aaron, just add additional tests, if you think that they would help to find issues.
What else do we need? The notes already store the stave that they are connected to. |
@0xfe is there a problem with this PR? I have the feeling that I have not been able to explain properly its aim. I also fixes the beams 🙂 |
@0xfe I think we should merge it. It doesn't break anything, it gives us some additional capabilities. I don't think it introduces any unnecessary complexity in the code or the interface. So....why not? I've already merged the change into Smoosic branch, but I haven't figured out how to work x-stave in the Smoosic UI yet. |
@AaronDavidNewman I contacted Mohit by email and he confirmed that with your go I can merge, let us do it! |
fixes #1373
ready for review now with more focus. It includes the tests from @AaronDavidNewman