Voice cross stave support in System

[rvilarl]

fixes #1373
ready for review now with more focus. It includes the tests from @AaronDavidNewman

[rvilarl]

[rvilarl]

Ready for review

[0xfe]

Thanks Rodrigo. This is good start. Re: the first two examples, are they intentionally off?

[AaronDavidNewman]

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.

[sschmidTU]

@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.

[rvilarl]

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.

[rvilarl]

Thanks Rodrigo. This is good start.

@0xfe Well this is not really a start, this started 5mo ago with #1373 there was also anther previous PR. It would be nice to get it through. I am happy to make changes if required.

[rvilarl]

Let us progress with this one this weekend 💪 and make a new release 🥳 !

[rvilarl]

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 :)

[rvilarl]

Current output with the beam code change:

[rvilarl]

Ready for review :)
@AaronDavidNewman #97 has some test cases that you might want to add to crossbeams tests

[rvilarl]

@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.

[AaronDavidNewman]

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.

[rvilarl]

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.

Hi Aaron, just add additional tests, if you think that they would help to find issues.

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.

What else do we need? The notes already store the stave that they are connected to.

[rvilarl]

@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 🙂

[AaronDavidNewman]

@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.

[rvilarl]

@AaronDavidNewman I contacted Mohit by email and he confirmed that with your go I can merge, let us do it!