search

0xfe / vexflow

A JavaScript library for rendering music notation and guitar tablature.
http://www.vexflow.com
Other
3.91k stars 662 forks source link

Rendering Stave Modifiers between Notes #23

Closed zz85 closed 10 years ago

zz85 commented 13 years ago

It seems that clefs/ other stave modifiers are currently rendered separately from notes/voices.

Is there a way to render a clef/key signature in between notes?

For example

[ "Treble" "D major" "4/4" c d e f "Bass" e f .... ]

Right now, all stave modifiers gets rendered to the left of the notes.

0xfe commented 13 years ago

Does this ever happen in-between bars/measures? (Ideally, you would render one stave per measure.)

zz85 commented 13 years ago

Yes, this would happen between bars especially on instruments like the cello or viola, when the ranges of notes go high above the stave, the clef would change eg. treble for viola, tenor for cello.

I'm not sure what are the best practices to use vexflow but I thought that 1 stave could be used continuously for an instrument (until it breaks line)

following the vexflow tutorial to layout notes, you have

var voice = new Voice();
voice.addTickable(notes);
stave.addClef("treble");
voice.addTickable(notes2);

formatter.FormatAndDraw(ctx, stave, notes);

it looks like the formatter has no knowledge of the stave's modifiers and vice-versa. would there be a plan to do so, or it would be better to break up the stave, and join a new stave everytime a clef change/key change is in between?

0xfe commented 13 years ago

In this case, I think the right thing to do here would be to make the clef/time signatures descend from StaveNote so that they can be laid out dynamically by the formatter. I'll look into it.

On Wed, Aug 31, 2011 at 10:39 PM, zz85 < reply@reply.github.com>wrote:

Yes, this would happen between bars especially on instruments like the cello or viola, when the ranges of notes go high above the stave, the clef would change eg. treble for viola, tenor for cello.

I'm not sure what are the best practices to use vexflow but I thought that 1 stave could be used continuously for an instrument (until it breaks line)

following the vexflow tutorial to layout notes, you have

var voice = new Voice();
voice.addTickable(notes);
stave.addClef("treble");
voice.addTickable(notes2);

formatter.FormatAndDraw(ctx, stave, notes);

it looks like the formatter has no knowledge of the stave's modifiers and vice-versa. would there be a plan to do so, or it would be better to break up the stave, and join a new stave everytime a clef change/key change is in between?

-- Reply to this email directly or view it on GitHub: https://github.com/0xfe/vexflow/issues/23#issuecomment-1964313

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

zz85 commented 13 years ago

That's great. It makes sense for the formatter to handle clef and time signatures too. Thanks Mohit!

LarryKu commented 13 years ago

Mohit, have you done any work towards deriving the stave modifiers from note.js? Greg Jopa just revised his tutorial update to utilize the recent barline & formatter modifications. He mentioned that the REPEAT_BEGIN does not work when used in conjunction with the barnote class. After looking into it, I saw that barnote seems to be designed to only draw barlines in a stave.

I'm beginning to think that all stave modifiers clef, time sig, key sig, bar lines, etc. should be derived from note.js and handled by the formatter. Before I start looking into this, I would like to hear other opinions. If they are included in the tickcontext with the proper left shift, I believe the formatter would handle them with little or no modification.

Larry

gregjopa commented 13 years ago

I created a new issue that describes this problem with barlines in more detail. Larry makes a good point that perhaps all stave modifiers should be derived from note.js. I support this idea and think this would provide a cleaner syntax for defining all notes and stave modifiers in single array for a voice.

0xfe commented 13 years ago

TOn Wed, Nov 9, 2011 at 11:52 AM, Larry Kuhns < reply@reply.github.com>wrote:

Mohit, have you done any work towards deriving the stave modifiers from note.js? Greg Jopa just revised his tutorial update to utilize the recent barline & formatter modifications. He mentioned that the REPEAT_BEGIN does not work when used in conjunction with the barnote class. After looking into it, I saw that barnote seems to be designed to only draw barlines in a stave.

I'm beginning to think that all stave modifiers clef, time sig, key sig, bar lines, etc. should be derived from note.js and handled by the formatter. Before I start looking into this, I would like to hear other opinions. If they are included in the tickcontext with the proper left shift, I believe the formatter would handle them with little or no modification.

Agreed. All of these should be positioned dynamically by the formatter, and the cleanest way to do this is to derive them from the Note class.

Mohit.

Larry

Reply to this email directly or view it on GitHub: https://github.com/0xfe/vexflow/issues/23#issuecomment-2683550

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

LarryKu commented 13 years ago

I had some time tonight, so for proof of concept, I hacked stave.js and converted stavebarline.js to be derived from note.js. Here is the result on the second method in Greg's tutorial example:

http://el-kay.com/test/VexTab/tests/flow2.html

There is still work on both classes, especially setting the barline width correctly depending on the barline type. It certainly won't work yet when concatenating staves (tutorial method 1).

Larry

gregjopa commented 13 years ago

Larry - your test looks good with formatting barlines as notes.

How would the formatting be done when a barline is added to a stave w/ setBegBarType() and setEndBarType()? I started working on updating these methods and if a barline isn't derived from stave modifier it is going to be difficult to format next to clefs and notes at the time the stave is drawn. I'm starting to think that these methods are unnecessary.

If bar lines, clefs, time signatures, and key signatures are going to be derived from the Note class do we even need to support methods for adding these directly to the staff? This would eliminate methods like Stave.addClef() and KeySignature.addToStave() and instead they would be added to a voice as notes. I know this is a major change to the api but Vexflow is still considered alpha and this would simplify the stave design and allow for more flexibility with stave modifiers which zz85 mentioned is needed for other instruments.

LarryKu commented 13 years ago

Greg,

Although I can't speak for Mohit, I believe that drawing clefs, time sigs, barlines, etc. has to be one way or the other and not both. The stave should function as a holder for musical objects the primary being clef, time sig, key sig, bar lines and notes. Other items you see drawn in a staff are modifiers either to bar lines or notes. One could also make a case for a measure being a compound object that encapsulates any or all of the above objects.

I currently have clefs, time signatures and bar lines being drawn dynamically as derivatives of the note class with a fair degree of success. The key sig will be a much more difficult task. The main thing that needs worked out is how and when to initialize start_x in the stave class so the available space for note formatting can be determined properly.

At least that's how I'm seeing it for now. I may change my mind once I get into it a bit more.

Larry

gregjopa commented 13 years ago

Glad to hear you’re working on these changes. Let me know if there’s anything I can do to help.

Just to clarify, since clefs, barlines, time sigs, and key sigs will be derived from the Note class all of these music objects will be passed to the formatter. So the new Vexflow syntax is going to look something like:

var stave = new Vex.Flow.Stave(10, 0, 500);

var musicObjects = [

new Vex.Flow.Barline("repeat_begin"),

new Vex.Flow.Clef("treble"),

new Vex.Flow.KeySignature("C"),

new Vex.Flow.TimeSignature("4/4"),

new Vex.Flow.StaveNote({ keys: ["c/4"], duration: "q" }), new Vex.Flow.StaveNote({ keys: ["d/4"], duration: "q" }), new Vex.Flow.StaveNote({ keys: ["b/4"], duration: "qr" }), new Vex.Flow.StaveNote({ keys: ["c/4", "e/4", "g/4"], duration: "q" }),

new Vex.Flow.Barline("repeat_end") ];

Vex.Flow.Formatter.FormatAndDraw(ctx, stave, musicObjects);

LarryKu commented 13 years ago

Greg,

Yes that would be the general syntax for engraving music. Once I get the key sig converted and determine the true affect on formatting, I'll let you know of anything you can do to help. I decided yesterday to create all objects as separate classes, so currently both methods currently will work. I don't think this is the best approach, but it does help my sanity while working on it.The question becomes, what direction Mohit wants for vexflow?

Larry

0xfe commented 13 years ago

On Fri, Nov 11, 2011 at 8:08 AM, Larry Kuhns < reply@reply.github.com>wrote:

Greg,

Yes that would be the general syntax for engraving music. Once I get the key sig converted and determine the true affect on formatting, I'll let you know of anything you can do to help. I decided yesterday to create all objects as separate classes, so currently both methods currently will work. I don't think this is the best approach, but it does help my sanity while working on it.The question becomes, what direction Mohit wants for vexflow?

I think supporting the new approach and deprecating the old one is the way to go. We can update the documentation and the VexTab code to use the new approach.

Larry

Reply to this email directly or view it on GitHub: https://github.com/0xfe/vexflow/issues/23#issuecomment-2707880

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

LarryKu commented 12 years ago

Here is an update on the new approach.

http://el-kay.com/test/VexTab/tests/flow3.html

If you scroll to the bottom of the page, the new approach is used for both non-justified and justified tests. I also show both tests with rectangles drawn around each element that I used for debugging. Not sure why the rectangles in the non-justified test are overlapping. Haven't had time to track it down.

In the tests for the old approach, I did lose the padding at the beginning of the measure, but I'm sure that can be resolved.

Larry

gregjopa commented 12 years ago

Larry – the new classes look great. Nice work!

I checked out the overlapping issue with the rectangles and I don’t think drawing rectangles around each music object is a good debugging tool for the formatter since the rectangles use inconsistent line widths when using floating point numbers as coordinates. You can notice this problem more with your tests if you prevent the rectangles from overlapping by taking into account the lineWidth by subtracting two from the rectangle width (ex: this.context.rect(x, debug_top, debug_width -2, debug_bottom - debug_top);) Then you will notice the right border of some of the rectangles do not have a consistent width. I created this jsFiddle that demonstrates this problem more clearly: http://jsfiddle.net/GregJ/EGdwW/

Perhaps a better way to test the formatter is to calculate what the width of the next tick should be based on the previous tick. This doesn’t need to be a visual test. It could be done by adding the x-coordinate, width, and padding of a tick from the formatter object (check out formatter.tContexts.map to get these values) and making sure the following tick’s x-coordinate matches that sum. I ran this test for a few ticks and noticed that since we are not restricting the number of decimal places there are floating point number errors occuring. For example, if you check out the width of the key signature (3rd tick) in your test w/ the new classes the width is 55.484700000000004 when it should be 55.4847. This floating point problem is common and is described here: http://stackoverflow.com/questions/1458633/elegant-workaround-for-javascript-floating-point-number-problem

This error isn’t noticeable since it’s such a small fraction of a pixel but I think it would be a good idea to limit the number of decimal places used in the formatter calculations. What do you think is the best way to resolve these floating point errors?

LarryKu commented 12 years ago

Greg,

I'm pretty happy about the new classes. They do work well. I'm thinking about adding an additional spacer object class, that would add the space after the barlines, and removing the current padding to the right of the barlines. This would enable the repeat barlines to be placed adjacent to each other for the repeat-end/repeat-begin combination. The other alternative is to add another barline type that would draw the compound repeat barline. It would be nice to get some feedback on this though.

I am not planning on keeping the debugging rectangles. I spent a couple hours trying to track down why the justified measures were leaving quite a bit of space at the right edge of the staff. So I could see visually what was happening, I added the debug rectangles. Only then did I realize that I had a stave width of 500 and a justify width of 420. It was one of those OMG moments. As soon as I set them the same, it came out perfect, and there was no bug !!!

If you look at the formatter, It does basically what you mentioned. I calculate the initial next tick position by adding the previous tick width to x, then moving the next tick-x to the right as needed to account for left modifiers. One thing the debug rectangles shows nicely is how the white space from the previous tick is used for left modifiers where ever possible to avoid right shifting the next tick. You can see this in the sharps on beats 2 & 3 where they are drawn in the previous tick white space.

The latest tests with the sharp in measure 1 also show how it moves notes to the right as needed and thus moves the end bar beyond the end of the staff. This was caused by the sharp on beat 1. I have been giving some thought to a method for forcing the justified notes to fit into the initial justify width that would require a secondary pass through the tickcontexts and applying a proportional adjustment. Haven't done anything with it though. It may work for small adjustments, but might create a big mess if a large amount of space must be recaptured.

I understand what you are saying about the floating point number problem. It has been a source of problems since day one. Mohit should probably provide his preferences on this.

Larry

gregjopa commented 12 years ago

I vote for the additional spacer object class for placing repeat barlines adjacent to each other and doing other barline formatting. This way the Vexflow syntax stays self-describing with using two separate barlines instead of a single compound repeat barline.

Thanks for explaining the details with how the formatter works with modifiers. Doing a second pass through to apply a proportional adjustment to handle modifiers sounds like a good idea. It would be great if the formatter would always fit a voice onto a staff without it overflowing, and if there isn’t enough space it throws an error.

Also, I just noticed the setBarNumber() method for barlines. Very Nice!

gregjopa commented 12 years ago

For the floating point number problem I have been playing around with the following function which rounds to precision:

Vex.Flow.TickContext.prototype.roundFloat = function(value, precision) { var power = Math.pow(10, precision); // Multiply up by precision, round accurately, then divide return Math.round(value * power) / power; };

Perhaps we can add it to tickcontext.js and use it in the setX() and setTickWidth() methods. Ex:

Vex.Flow.TickContext.prototype.setX = function(x) { this.x = this.roundFloat(x, 10); return this; }

zz85 commented 12 years ago

i think we did touch a little about rational numbers here

i think its not difficult to implement a fraction class (eg https://github.com/ekg/fraction.js) so less precision would be lost (compared to rounding for example). i could write a quick fraction class for this purpose.

0xfe commented 12 years ago

Hi Larry,

Firstly, the demos look very good. Good job.

Re: spacer objects, there is already a GhostNote class that implements a simple spacer. See if that works for you.

Re: rectangles -- I really like them for debugging. I resort to things like that too when debugging my layouts at a high level, they are great visual cues for glaring errors that are hard to catch programmatically. That said, writing unit tests to verify ticks and widths is also necessary. So, if you can break out the debug-rectangle code into a separate unit/file/class, I think it would be very useful.

Re: floating point. There are two parts to this: ticks and coordinates.

Anyhow, I love what you've done so far. Keep 'em coming.

Mohit.

(Also, nice touch on the bar numbers.)

On Sun, Nov 13, 2011 at 6:40 PM, Greg Jopa < reply@reply.github.com

wrote:

Larry the new classes look great. Nice work!

I checked out the overlapping issue with the rectangles and I dont think drawing rectangles around each music object is a good debugging tool for the formatter since the rectangles use inconsistent line widths when using floating point numbers as coordinates. You can notice this problem more with your tests if you prevent the rectangles from overlapping by taking into account the lineWidth by subtracting two from the rectangle width (ex: this.context.rect(x, debug_top, debug_width -2, debug_bottom - debug_top);) Then you will notice the right border of some of the rectangles do not have a consistent width. I created this jsFiddle that demonstrates this problem more clearly: http://jsfiddle.net/GregJ/EGdwW/

Perhaps a better way to test the formatter is to calculate what the width of the next tick should be based on the previous tick. This doesnt need to be a visual test. It could be done by adding the x-coordinate, width, and padding of a tick from the formatter object (check out formatter.tContexts.map to get these values) and making sure the following ticks x-coordinate matches that sum. I ran this test for a few ticks and noticed that since we are not restricting the number of decimal places there are floating point number errors occuring. For example, if you check out the width of the key signature (3rd tick) in your test w/ the new classes the width is 55.484700000000004 when it should be 55.4847. This floating point problem is common and is described here: http://stackoverflow.com/questions/1458633/elegant-workaround-for-javascript-floating-point-number-problem

This error isnt noticeable since its such a small fraction of a pixel but I think it would be a good idea to limit the number of decimal places used in the formatter calculations. What do you think is the best way to resolve these floating point errors?

Reply to this email directly or view it on GitHub: https://github.com/0xfe/vexflow/issues/23#issuecomment-2725271

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

gregjopa commented 12 years ago

Awesome feedback. I have definitely learned a lot from this thread.

Re: spacer class, I think this would be used to remove unwanted padding added by the formatter to certain barlines. For example, in Guitar Pro when I add a repeat end barline next to a repeat begin barline these two barlines are formatted back to back with no padding in between. Also, with the repeat end barline I think it would be awesome to add the number of play times above it. Guitar Pro adds this for any repeat played more than twice so for playing a repeat 4 times it places “4x” above the repeat close. Perhaps it would be better to name this class barline-wrapper.

I can work on creating some tests for the formatter. I think Larry’s rectangle drawing tests could be broken out and drawn using the coordinates of each tick stored in the formatter.tContexts.map. I didn’t notice any attribute for tick type in this map so I am not sure how we would draw different colors around the different music objects. Would it make sense to add a type attribute to tickContext.js to define the music object type?

0xfe commented 12 years ago

On Wed, Nov 16, 2011 at 1:04 AM, Greg Jopa < reply@reply.github.com

wrote:

Awesome feedback. I have definitely learned a lot from this thread.

Re: spacer class, I think this would be used to remove unwanted padding added by the formatter to certain barlines. For example, in Guitar Pro when I add a repeat end barline next to a repeat begin barline these two barlines are formatted back to back with no padding in between. Also, with the repeat end barline I think it would be awesome to add the number of play times above it. Guitar Pro adds this for any repeat played more than twice so for playing a repeat 4 times it places 4x above the repeat close. Perhaps it would be better to name this class barline-wrapper.

I can work on creating some tests for the formatter. I think Larrys rectangle drawing tests could be broken out and drawn using the coordinates of each tick stored in the formatter.tContexts.map. I didnt notice any attribute for tick type in this map so I am not sure how we would draw different colors around the different music objects. Would it make sense to add a type attribute to tickContext.js to define the music object type?

I would prefer not to use this approach because 1) we'd be coupling a debugging tool with a core class (the tickcontext does not need to know the type), and 2) we'd have to worry about keeping the type attribute in sync with the objects in the context (which would be quite intrusive.)

How about simply scanning the tickContext's list of "tickables" and deduce the color from there?

Mohit.

Reply to this email directly or view it on GitHub: https://github.com/0xfe/vexflow/issues/23#issuecomment-2756109

Mohit Muthanna [mohit (at) muthanna (uhuh) com]

gregjopa commented 12 years ago

That sounds good. I didn’t realize that there’s already a tickType attribute in tickable.js and Larry’s new code sets this value for each type of music object. I will start working on these formatter tests.

LarryKu commented 12 years ago

I thought I had better get up-to-date on this thread. I've ha a touch of the flu and didn't fell good enough to mess with the computer.

Re: Spacer - On second thought, this would require a spacer after each barline, which is not a desirable approach. A better approach is to set the tickcontext padding to "0" for barlines, and then set the barline width plus padding as needed in the barlineobject class. Thus when you have a end-repeat / begin-repeat pair, the end-repeat padding would be "0", and the barlines would drawn correctly.

Drawing the repeat x-times text should be simple enough to implement. It is very similar to the bar numbers.

I introduced the tickType variable in the tickable class to enable the sorting of the tickcontext map indexes such that for a given tick count, the order would be clef/time-sig/key-sig/bar line/notes. How the index is created is a bit awkward, but is does work.

Drawing the bounding boxes could easily be added to Vex.js. This class already contains debug functions, and seems to the the logical place for stave bounding boxes.

Larry

0xfe commented 10 years ago

This is fixed.