Add size optimization for SCJingleConverter. #18

different55 · 2020-01-31T05:02:41Z

First time screwing around in C++ so definitely needs some work, any and all advice for cleanup would be welcome.

First time screwing around in C++ so definitely needs some work.

different55 · 2020-01-31T05:10:32Z

Basically it adds a "isTied" boolean to notes. It's set when a note is tied or if it's a rest, because they can all be tied together. I don't know if this is MusicXML spec or MuseScore convention, but even ties that stretch from note to note are marked as starting and stopping on each note, so we only have to pay attention to starting tie tags.

Then when we make our optimization pass, we step backwards through a concatenated list of notes, combining each tied note with the following one as long as the pitch matches.

Trailing rests get removed, and if there is leading silence that's trimmed, too. From either or both channels if possible.

greggersaurus · 2020-02-10T02:49:06Z

@different55 Just wanted to say thanks for the pull request and let you know that I am looking at it (sorry for delay and silence up to this point). Hoping to be back with something more concrete soon!

greggersaurus · 2020-02-20T07:22:17Z

@different55 can you point me to (or attach) what songs you were testing with? I like what you've done here, but I want to experiment with a slightly different code structure (incorporate optimizeNotes() into download() so that optimizeNotes() doesn't need to be called every time a setting is changed and so that optimizedNotesL/optimizedNotesR aren't required). However, before I share my proposed differences with you I want to make sure I'm not overlooking anything and my changes produce the same results as yours.

different55 · 2020-02-20T20:35:23Z

Got together a few songs that optimize pretty well and have a lot of variety. Got tied notes, tied notes that cross measures, consecutive rests, leading silence when there's only one channel, leading silence only on one channel, trailing silence, only thing I'm missing is is leading silence on two channels.

Testing music.zip

different55 · 2020-02-20T20:37:01Z

IIRC I started out with it in download() but I eventually split it off into its own thing so that I could keep the memory usage counter accurate.

greggersaurus · 2020-02-25T05:37:52Z

Jingle/SCJingleConverter/composition.h

        {}
        std::vector<float> frequencies; // Frequency of note in Hz. Could be multiple
            // frequencies in case of chord. Frequencies to be sorted into descending
            // order at end of parse.
        float length; // Number of beats that Note/Chord lasts for
+        bool tied; // Whether or not this note tries to be tied to the next.


Change description to something like "True if note is tied to next (i.e. single unbroken note)"

greggersaurus · 2020-02-25T05:39:06Z

Jingle/SCJingleConverter/composition.h

@@ -265,6 +269,8 @@ class Composition
        // The QString Key is of the Form "Part {n} Voice {m}" where n is the Part {n} is the
        // part id (with {n} changing for different parts) and Voice {m} designates
        // which voice within the part we are referring to.
+    std::vector<Note> optimizedNotesL;


Add descriptions for these. Why do they exist and how do they relate to voices.

greggersaurus · 2020-02-25T05:40:51Z

Jingle/SCJingleConverter/composition.cpp

+        }
+    }
+
+    // Remove leading silence


Add a GUI control to make this optional. I'm all for changing the internal structure to save space, but don't want to force any timing changes upon a user (maybe some people want preceding silence?).

greggersaurus · 2020-02-25T05:44:24Z

Jingle/SCJingleConverter/composition.cpp


+    // Optimize Notes in Right Channel
+    if (voices.count(voiceStrR)) {


This is similar enough to "// Optimize Notes in Left Channel" block above and code is complex enough that this should be a function (e.g. optimizeNotes(Channel chan)).

greggersaurus · 2020-02-25T05:45:55Z

Jingle/SCJingleConverter/composition.cpp

+        }
+
+        // Combine ties
+        for (uint32_t notes_idx = optimizedNotesR.size()-1; notes_idx-- > 0; ) {


notes_idx should not be decremented in comparison part of for loop

Which would fit better?

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx < optimizedNotes.size(); notes_idx--) { ...

Still kinda ugly, now it'll wrap around, but it still works.

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; ) { notes_idx--; ....

Or this only pulls the notes_idx-- down out of the comparison.

Currently leaning towards the second.

First, the logic in your first code block doesn't match the others. Was it a mistake that the condition check part of the for loop is notes_idx < optimizedNotes.size() and not notes_idx > 0 or am I missing something?

Second, I'm less worried with text wrap around (assuming I understand your "ugly" comment) and more about code readability in terms functionality. The ++/-- features in C++ is great, but when it gets put into parts of a statement where people might not usually look for it, my opinion is that that code is hard to read and error prone if anyone were to change it. So I would think something like the following would be acceptable:

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; notes_idx--) {

Saying all that though, I also have an addendum. Is it possible we can get to this code and optimizedNotesR.size() will be zero? In that case, since notes_idx is unsigned, we will get wrap around and out of bounds issues. Initial thought is to change the for loop to count up and use that count value to compute the access index. Thoughts?

First, the logic in your first code block doesn't match the others. Was it a mistake that the condition check part of the for loop is notes_idx < optimizedNotes.size() and not notes_idx > 0 or am I missing something?

Sorry, wrapping around is referring to integer underflow. If I use > 0, the first two notes will never be optimized since the loop never runs when notes_idx == 0. Then the compiler immediately yelled at me for trying notes_idx <= 0 on an unsigned int, which led me to this. In this code, notes_idx < optimizedNotes.size() will evaluate false when notes_idx underflows and becomes greater than optimizedNotes.size(), which is kinda ugly.

Totally get what you're saying about putting things in unexpected places.

for (uint32_t notes_idx = optimizedNotes.size()-1; notes_idx > 0; notes_idx--) {

Has the problem I touched on earlier, the loop will never run when notes_idx == 0 so the first note (first two, sorta) get ignored and never optimized. Moving the notes_idx-- to be the first statement in the loop is functionally identical to the weird notes_idx-- > 0 without being as bad for readability. It allows the rest of the loop to run while notes_idx == 0, without having to deal with that in the conditional, either by decrementing it in an unnatural place or unintuitively dealing with underflow.

Saying all that though, I also have an addendum. Is it possible we can get to this code and optimizedNotesR.size() will be zero? In that case, since notes_idx is unsigned, we will get wrap around and out of bounds issues.

Can't quite tell. It checks to see if the current active voice exists, then collects all notes from all measures in that voice into optimizedNotes. Are voices guaranteed to have at least some notes in them, even rests? As long as it has something, even if it's 100% rests we're good to go.

Initial thought is to change the for loop to count up and use that count value to compute the access index. Thoughts?

You say using the count value to compute the access index... are you saying the loop would count up while the actual index accessed would still be counting backwards or? I'm having a hard time wrapping my head around that lol.

I was thinking something like the following maybe:

for (uint32_t notes_cnt = 1; notes_cnt < optimizedNotesR.size(); notes_cnt++) { uint32_t notes_idx = optimizedNotesR.size() - notes_cnt; ...

In this way we don't have to worry about whether optimizedNotesR.size() can ever be zero, because if it is we will never enter the loop to begin with.

Thoughts?

I haven't tested it yet but my initial thought is that something like that would be immediately thrown off once we start pulling notes off the end as they're optimized out, unless we add something to adjust the count every time that happens. If the goal's just to be sure that it's not empty, seems like making a separate, explicit check for that would be clearer.

If the goal is just to make it run at == 0, then use a signed integer as the index var. As long as the index isn't larger than INT_MAX, there won't be any weird wrapping, and you can add asserts to that effect if you are worried about it. Also since you intentionally don't want to process the very last node first (because you are comparing with the next node), you can start at - 2 and remove the < size - 1 check below.

for (int notes_idx = optimizedNotes.size() - 2; notes_idx >= 0; notes_idx--) {

greggersaurus · 2020-02-25T05:50:36Z

@different55 thanks for pointing out the desire to keep the memory usage up to date and accurate. Given that I think your approach for having an optimizeNotes() function that is called whenever a user makes a configuration change is necessary. I've reviewed the code. Please address the comments I've made above. Thanks!

NoahRic

Drive-by!

NoahRic · 2020-03-04T04:03:18Z

Jingle/SCJingleConverter/composition.cpp

+            Measure& meas = voices[voiceStrL].measures[meas_idx];
+            for (uint32_t notes_idx = 0; notes_idx < meas.notes.size(); notes_idx++) {
+                optimizedNotesL.push_back(meas.notes[notes_idx]);
+            }


Assuming meas.notes is a vector, you can replace this loop with:

optimizedNotesL = meas.notes

It is, but I don't think we can. We're appending multiple measures to optimizedNotesL.

Can't reply to the comment on composistion.cpp for some reason but that sounds good to me. For some reason thought we were limited to uint32_t or maybe just unsigned ints. In hindsight that would have been a pretty easy thing to change and test lol.

Oops, sorry, missed the context! In that case, does std::vector::insert help?

optimizedNotesL.insert(optimizedNotesL.end(), meas.notes.begin(), meas.notes.end());

Looks pretty great to me, yeah!

NoahRic · 2020-03-04T04:03:46Z

Jingle/SCJingleConverter/composition.cpp

+        }
+
+        // Combine ties
+        for (uint32_t notes_idx = optimizedNotesR.size()-1; notes_idx-- > 0; ) {


If the goal is just to make it run at == 0, then use a signed integer as the index var. As long as the index isn't larger than INT_MAX, there won't be any weird wrapping, and you can add asserts to that effect if you are worried about it. Also since you intentionally don't want to process the very last node first (because you are comparing with the next node), you can start at - 2 and remove the < size - 1 check below.

for (int notes_idx = optimizedNotes.size() - 2; notes_idx >= 0; notes_idx--) {

Add size optimization for SCJingleConverter.

0174cc0

First time screwing around in C++ so definitely needs some work.

greggersaurus reviewed Feb 25, 2020

View reviewed changes

NoahRic reviewed Mar 4, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add size optimization for SCJingleConverter. #18

Add size optimization for SCJingleConverter. #18

different55 commented Jan 31, 2020

different55 commented Jan 31, 2020

greggersaurus commented Feb 10, 2020

greggersaurus commented Feb 20, 2020

different55 commented Feb 20, 2020

different55 commented Feb 20, 2020

greggersaurus Feb 25, 2020

greggersaurus Feb 25, 2020

greggersaurus Feb 25, 2020

greggersaurus Feb 25, 2020

greggersaurus Feb 25, 2020

different55 Feb 26, 2020

greggersaurus Mar 2, 2020

different55 Mar 2, 2020

greggersaurus Mar 3, 2020

different55 Mar 3, 2020

NoahRic Mar 4, 2020

greggersaurus commented Feb 25, 2020

NoahRic left a comment

NoahRic Mar 4, 2020

different55 Mar 4, 2020

different55 Mar 4, 2020

NoahRic Mar 4, 2020

different55 Mar 6, 2020

NoahRic Mar 4, 2020


		// Optimize Notes in Right Channel
		if (voices.count(voiceStrR)) {

Add size optimization for SCJingleConverter. #18

Are you sure you want to change the base?

Add size optimization for SCJingleConverter. #18

Conversation

different55 commented Jan 31, 2020

different55 commented Jan 31, 2020

greggersaurus commented Feb 10, 2020

greggersaurus commented Feb 20, 2020

different55 commented Feb 20, 2020

different55 commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greggersaurus commented Feb 25, 2020

NoahRic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment