0xfe / vexflow

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

Flow.keyProperties hangs in certain contexts #1595

Closed jaredjj3 closed 9 months ago

jaredjj3 commented 9 months ago

TL;DR: Doing arithmetic using -0 seems to vary by platform. I recommend that Table.keyProperties to be updated to remove one of its instances.

vexflow-key-properties-issue is the reproduction repo, which also has a README that describes the symptoms and how to run the examples.

@rvilarl and I have been working on a MusicXML -> VexFlow library called vexml. I'm having trouble with https://github.com/stringsync/vexml/pull/96. It's a really esoteric issue that seems orthogonal to vexflow, but I figured I'd reach out to see if you all could find something I'm doing wrong.

I observed that Flow.keyProperties cannot be called >O(1000) times in a test run (example), depending on the platform. I narrowed down the issue to this line in Table.keyProperties.

If I change the implementation from

octave += -1 * options.octave_shift;

to

octave -= options.octave_shift;

the tests work. I suspect that it's some issue propagating -0 on some platforms. However, when I explode out the operations explicitly, I'm unable to reproduce it.

Would you accept a PR to make the change I mentioned?

rvilarl commented 9 months ago

Of course we would. Thanks Jared!

ronyeh commented 9 months ago

🤯

Wow. Thanks for the catch and fix. If you ever figure out why this occurs, I'd love to learn about it.