Saving measures' BPM and fix #557 #558
Conversation
| @@ -142,6 +142,7 @@ export class ExpressionReader { | |||
| if (tempoAttr) { | |||
| const match: string[] = tempoAttr.value.match(/\d+/); | |||
| this.soundTempo = match !== undefined ? parseInt(match[0], 10) : 100; | |||
| currentMeasure.TempoInBPM = this.soundTempo; | |||
There was a problem hiding this comment.
Sound is an optional attribute. So your tempo might always be 0.
If somebody implementing a midi player wants to use this it might come in handy to set the tempo to 120 as a default?
There was a problem hiding this comment.
I think we should introduce a flag, to tell the user if the bpm was ever set in the sheet, or its value was not specified
| @@ -32,6 +32,7 @@ export class SourceMeasure { | |||
| this.endingBarStyle = ""; | |||
| this.firstInstructionsStaffEntries = new Array(completeNumberOfStaves); | |||
| this.lastInstructionsStaffEntries = new Array(completeNumberOfStaves); | |||
| this.TempoInBPM = 0; | |||
There was a problem hiding this comment.
Oh yeah here is the 0. Maybe make it! = 0
There was a problem hiding this comment.
Gonna update this to 120
| @@ -505,6 +505,10 @@ export class InstrumentReader { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| if (currentMeasure.TempoInBPM === 0) { | |||
| this.currentMeasure.TempoInBPM = this.previousMeasure.TempoInBPM; | |||
There was a problem hiding this comment.
If having a fix value for the tempo this is unnecessary
There was a problem hiding this comment.
How do I distinguish between fix and dynamic BPM? What I have in mind is upon playing each measure I would do something like setAudioPlayBackBPM(currentMeasure.TempoInBPM) be it a fix or dynamic value
There was a problem hiding this comment.
You're right, having thought about that. My comment was about the initial value. So in your case that is correct.
|
The field you are referring to might be from our old implementation which we ported to JS. Maybe reuse it as a default for your implementation, see PR comments |
|
@bneumann what about |
Test 1 :No Test 2Varying BPM Test 3
|
|
Hi! Just to clarify the original idea of the code and the variables: |
|
@matt-uib thank you very much for the clarification, in that case, maybe I should set |
|
Yes that would make sense. And then, if userStartTempoInBPM has not been set by the user (e.g. =0) , which will currently be always the case, set it as well by copying the value from defaultTempoInBpm. defaultTempoInBpm shall then never be changed and userStartTempoInBPM can be changed by the user. |
|
@matt-uib something like this? this.musicSheet.DefaultStartTempoInBpm = this.musicSheet.SourceMeasures[0].TempoInBPM;
this.musicSheet.userStartTempoInBPM = this.musicSheet.userStartTempoInBPM || this.musicSheet.DefaultStartTempoInBpm; |
|
The last commit improves reading tempo that appears inside of |
|
LGTM. seems like all suggestions were implemented, anything else can be changed later. |
praisethemoon strikes once again with another PR. I personally needed such API to capture measure's tempo.
Does this deprecate
this.osmd.sheet.userStartTempoInBPM? Right now it has a different value (100) and I think it either must be deprecated, or inherits its value from BPM of the first measure.Any thoughts? 🎵