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

Added feature MinMeasureToDrawIndex #528

Merged
merged 2 commits into from
May 30, 2019
Merged

Added feature MinMeasureToDrawIndex #528

merged 2 commits into from
May 30, 2019

Conversation

praisethemoon
Copy link
Collaborator

Hello,

I have added a new feature I personally wanted, allowing to render a particular interval of measures. Now, MaxMeasureToDrawIndex is already implemented. I simply wanted to add its opposite MinMeasureToDrawIndex.

There is one thing that I want to ask you, how should I handle the case where MinMeasureToDrawIndex > MaxMeasureToDrawIndex ? Currently, an exception TypeError: this.graphicalMusicSheet.MusicPages[0] is undefined is thrown automatically. Other than that, here is a screenshot of the feature in action.

Screenshot 2019-05-30 at 03 35 25

@bneumann
Copy link
Collaborator

bneumann commented May 30, 2019

Awesome work! I personally have no preference on how to handle the error. In the skybottom calculator I simple switched min and max if min > max but now you could argue if that is error tolerant programming or confusing 😂

Maybe @matt-uib has any preferences?

Other then that: let's get it merged

@sschmidTU
Copy link
Contributor

Awesome! I had tried to implement selecting a first and last measure to render, but i always had errors in functions that tried to access measures out of the range. You fixed two of them.

During testing i actually found a few more of these issues and fixed them. So i'll merge this PR to a feature branch and fix it up for merging to develop.

By the way, minMeasureToDrawIndex should be drawFromMeasureNumber - 1, because measure 1 has index 0.

@praisethemoon btw, is your nickname a Dark Souls/Bloodborne reference? Love it.
And thanks for submitting the PR, without it this wouldn't have been done for a while!

@sschmidTU
Copy link
Contributor

Oh and when the minimum measure to draw is greater than the max measure length, i just set it to max length - 1 and give a debug warning on console. otherwise we have 0 measures to draw and get the error you mentioned.
of course we could change that. drawing nothing would be the logical choice, but could be confusing.

@sschmidTU
Copy link
Contributor

I'll make a new release as well after this is merged, because we've collected a few nice improvements again.

@sschmidTU sschmidTU changed the base branch from develop to feat/drawFromMeasureNumber May 30, 2019 16:23
@sschmidTU sschmidTU merged commit ca473d1 into opensheetmusicdisplay:feat/drawFromMeasureNumber May 30, 2019
@praisethemoon
Copy link
Collaborator Author

@bneumann @sschmidTU I am glad you've found this PR useful!

@sschmidTU You got me 😅 I am a huge fan of Miyazaki's works

I have done a couple more tests and I have found two issues related to Cursors and Min/Max measure which I plan to fix although, i am very new to OSMD and TypeScript, so ideas are more than welcome ❤️ . Here are the issues I've found:

  1. Changing the MinMeasureToDrawIndex from 0 to any other value, then re-rendering osmd works just fine. However, when rendering the cursor, an exception is raised within the Cursor class with message TypeError: this.graphicalMusicSheet.MusicPages[0] is undefined, and the cursor wont be able to be updated any longer. However, if we redo the same process but first incrementing the cursor until it reaches the MinMeasureToDrawIndex measure, then changing it within osmd, it will work just fine.

  2. The same error applies to MaxMeasureToDrawIndex, when the cursor is in a measure that is higher than the specified maximum.

What is the best way to approach these two issues?

@sschmidTU
Copy link
Contributor

Ah, yes, i haven't tested the cursor with this yet, makes sense that it needs adjustments.
I'm just about to push my fixes so far for this PR to the new branch.
I'll look at the cursor after that.

ps: I'm a huge fan of Miyazaki's works as well 😄

@praisethemoon
Copy link
Collaborator Author

I will keep trying to fix it as well, it would help me learn the architecture and internal design of the library. Meanwhile, you should try Sekiro 🎮 😆

@sschmidTU
Copy link
Contributor

this will close #482 by the way, we'll make a lot of people happy by implementing this feature ;)

and yes, i'm dying to play Sekiro, just didn't get to it yet :)

sschmidTU added a commit that referenced this pull request May 30, 2019
… drawing specific range of measures

fix for PR #528, part of feature #482
@sschmidTU
Copy link
Contributor

sschmidTU commented May 30, 2019

my fixes (see above) are pushed to the branch feat/drawFromMeasureNumber,
take a look if you're interested.
it's even possible to draw any single measure from a piece now.
let's see if we can fix the Cursor as well. though for today this was it for me.

@sschmidTU
Copy link
Contributor

sschmidTU commented May 30, 2019

for testing, you can decommentate the lines in index.js.selectSampleOnChange(), which will draw a random range of measures each time a new sample is loaded.

@sschmidTU
Copy link
Contributor

@praisethemoon by the way, for quick (and/or random) chatting, there's also gitter ;)
https://gitter.im/opensheetmusicdisplay/opensheetmusicdisplay

@praisethemoon
Copy link
Collaborator Author

Thanks for tips and the invitation! Will try your suggestions :)

@sschmidTU
Copy link
Contributor

@praisethemoon I finally got around to merging in your work to develop with my fixes, sorry for the delay! release will be out soon as well. see #482

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.

3 participants