mB_prJune23_2020 #5544

musikBear · 2020-06-23T12:04:11Z

I have

Cleared repo-root for 2 duplicate files.
Changed logic for Instrument-envelopes, in order to preserve AFP-sample backward comparability.
Added 7 features:

General:

New piano-like envelope for general instrument (Part of Making a good first-view impression #2092

Piano-roll:

Humanize note position (speedbar-button) (Part of Per-instrument Timing, Volume and Pitch Humanization/Randomization #1165
Humanize note velocity (speedbar-button) (Part of Per-instrument Timing, Volume and Pitch Humanization/Randomization #1165
Strum from top (speedbar-button) (Chord delay #2380
Strum from bottom (speedbar-button) (Chord delay #2380
Shorten notes by 1/192 ( key - ) (key-combo to change note-length in piano-roll #3138
lengthen notes by 1/192 ( key + ) (key-combo to change note-length in piano-roll #3138

Methods demo: https://youtu.be/OlBtU0JYvyY

Changes to default envelope that removes clicks and pops for all instrument with envelopes

… void strumNotes(); Strum selected notes with fixed value

…trumNotes();

I have added 7 features General: 1. New piano-like envelope for general instrument Pianoroll 2. Humanize note position (speedbar-button) 3. Humanize note velocity (speedbar-button) 4. Strum from top (speedbar-button) 5. Strum from bottom (speedbar-button) 6. Shorten notes by 1/192 ( key - ) 7. lengthen notes by 1/192 ( key + ) Methods demo: https://youtu.be/OlBtU0JYvyY

removes erroneous added file

Removed include for random and debug

This update preserve all AFP-volume-envelopes and is fully backward compatible New objects from Sidebar| My-Instruments, will have a new more instrument-like std-envelope. It will default not be enabled.

New prototypes for humanizing & strumming in piano-roll

Adding methods for * Humanizing selected note-position & Note-velocity * Strumming selected chords from Up & Down * Moving selected notes with + & - in 1/192 steps

LmmsBot · 2020-06-23T12:09:37Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.2.1.714-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.1.714-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.2.1.714-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.2.1-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.2.1-msvc2017-win64.exe (build link)

macOS

macOS : lmms-1.2.1.714-mac10.13.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://6998-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.714-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6998?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://6995-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.714-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6995?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://6997-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.714-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6997?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ntc8w7xflvxudiwg/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33687049"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/n8xub8wn44vmewcw/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/33687049"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://6996-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.714-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/6996?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "03f8318bebfd17a5e103078a5cf30bbf841d5d87"}

SecondFlight

See specific review comments.

Also, please use tabs for formatting instead of spaces. It's a setting in Qt Creator. You will probably also need to manually replace your current formatting with tabs again, since I don't think Qt Creator will automatically change spaces to tabs when you change that setting.

See here for the LMMS style guide: https://github.com/LMMS/lmms/wiki/Coding-conventions

SecondFlight · 2020-06-23T16:15:16Z

src/core/EnvelopeAndLfoParameters.cpp

-				SECS_PER_LFO_OSCILLATION * 1000.0, this,
-							tr( "LFO frequency" ) ),
-	m_lfoAmountModel( 0.0, -1.0, 1.0, 0.005, this, tr( "LFO mod amount" ) ),
-	m_lfoWaveModel( SineWave, 0, NumLfoShapes, this, tr( "LFO wave shape" ) ),
-	m_x100Model( false, this, tr( "LFO frequency x 100" ) ),
-	m_controlEnvAmountModel( false, this, tr( "Modulate env amount" ) ),
+	SECS_PER_LFO_OSCILLATION * 1000.0, this,
+	tr( "LFO speed" ) ),
+	m_lfoAmountModel( 0.0, -1.0, 1.0, 0.005, this, tr( "LFO Modulation" ) ),
+	m_lfoWaveModel( SineWave, 0, NumLfoShapes, this, tr( "LFO Wave Shape" ) ),
+	m_x100Model( false, this, tr( "Freq x 100" ) ),
+	m_controlEnvAmountModel( false, this, tr( "Modulate Env-Amount" ) ),


Will updating these break translations?

I'm also not sure about the changes here - I like frequency -> speed, but I'm not as sure about env amount -> Env-Amount. This makes the capitalization inconsistent, as it hasn't been changed above (above is first word has first letter capitalized: 'Env attack', 'Env release', etc, while this is all words have first letter capitalized: 'LFO Modulation', 'LFO Wave Shape', etc).

Also, Env mod amount has not been changed, so I think changing LFO mod amount to LFO Modulation could be confusing to a new user - they both do the same thing for their respective modulators, i.e. control mod amount, so naming them in a similar way makes the most sense to me.

I would personally suggest leaving the names unchanged for this PR and submitting a separate PR for name changes that also maintains consistency across LMMS, and also that deals with any translation issues, if that's something you're interested in.

SecondFlight · 2020-06-23T16:18:11Z

src/core/EnvelopeAndLfoParameters.cpp

-	const float lafI = 1.0f / qMax( minimumFrames, m_lfoAttackFrames );
+	const float lafI = 1.0f / m_lfoAttackFrames;


I'm curious for your rationale behind removing the minimum here. Can you guarantee that m_lfoAttackFrames will never be 0?

Can you guarantee that m_lfoAttackFrames will never be 0?

He can't. I believe I added that statement early after stable-1.2 was branched off master. One of the things that most likely will break by this change is the fpe debug option (WANT_DEBUG_FPE). @musikBear What was the reason behind this change?

What was the reason behind this change?

I have N O T made these changes!
I do not have the faintest idea, why they are in this commit!
Could that happen if i updated one file through GIT?
Bottom-line : The only code i have added in EnvelopeAndLfoParameters.cpp is for the Envelopes, nothing else has been touched by me
My code:

// m_predelayModel( val, min, max, incr, this, tr( ">info<" ) ) m_predelayModel( 0.0, 0.0, 2.0, 0.001, this, tr( "Env pre-delay" ) ), m_attackModel( 0.025, 0.0, 2.0, 0.001, this, tr( "Env attack" ) ), m_holdModel( 0.0, 0.0, 2.0, 0.001, this, tr( "Env hold" ) ), m_decayModel( 0.6, 0.0, 2.0, 0.001, this, tr( "Env decay" ) ), m_sustainModel( 0.0, 0.0, 1.0, 0.001, this, tr( "Env sustain" ) ), m_releaseModel( 0.2, 0.0, 2.0, 0.001, this, tr( "Env release" ) ), m_amountModel( 1.0, -1.0, 1.0, 0.005, this, tr( "Env mod amount" ) ), //AMP

And ONLY those lines are mine.
This is my fault, no question, but HOW i have gotten an unknown file 'merged'(?) into my commit, i do not know. I used the 'add-file-through-upload'.
I definitively expected that my 'added' file would replace the current!
I does NOT?!?

This must be something with your git workflow then... I would expect this to show up as a merge conflict under a typical fork -> commit -> PR scenario.

My guess here is that there are changes in LMMS/lmms/master that you don't have yet. You may have to manually update your code with the changes in master.

I definitively expected that my 'added' file would replace the current!

You shouldn't add files. You should push commits. Your master branch is borked and needs to be reset to the lmms master branch.

I used the 'add-file-through-upload'.

What's that? Are you uploading files via the GitHub interface? Have you followed any tutorial to learn how to work with git/GitHub?

SecondFlight · 2020-06-23T16:20:29Z

src/core/EnvelopeAndLfoParameters.cpp

+	// ### TODO:
+/*	// Keep compatibility with version 2.1 file format
+	if( _this.hasAttribute( "lfosyncmode" ) )
+	{
+		m_lfoSpeedKnob->setSyncMode(
+		( TempoSyncKnob::TtempoSyncMode ) _this.attribute(
+						"lfosyncmode" ).toInt() );
+	}*/
+


Did you mean to leave this commented? Also curious if you meant to put 1.2 and not 2.1.

SecondFlight · 2020-06-23T16:24:30Z

src/gui/editors/PianoRoll.cpp

-		Engine::getSong()->playSong();
+
+        Engine::getSong()->playPattern( m_pattern );
+        Engine::getSong()->playSong();


I'm curious what this change does. My instinct says it would attempt to play both the pattern and the song when recording, which might cause the pattern to play twice in some cases? I don't know this code well.

SecondFlight · 2020-06-23T16:25:28Z

src/gui/editors/PianoRoll.cpp

-					tr( "Please enter a new value between %1 and %2:" ).
-						arg( MinVolume ).arg( MaxVolume ),
-					(*nv)[0]->getVolume(),
-					MinVolume, MaxVolume, 1, &ok );
+		tr( "Please enter a new value between %1 and %2:" ).arg( MinVolume ).arg( 
+            MaxVolume ),(*nv)[0]->getVolume(),MinVolume, MaxVolume, 1, &ok );


Could you revert this change? The old formatting is consistent with typical C++ formatting guidelines.

SecondFlight · 2020-06-23T16:30:12Z

src/gui/editors/PianoRoll.cpp

+	QAction* humanizeVelAction = 
+    new QAction(embed::getIconPixmap( "humaVel" ), tr( "Humanize Selected Note-Velocity" ), this );
+	connect( humanizeVelAction, SIGNAL( triggered() ), m_editor, SLOT( humanizeNotesVel() ) );
+
+    QAction* humanizeLenAction = 
+    new QAction(embed::getIconPixmap( "humaPos" ), tr( "Humanize Selected Note-position" ), this );
+	connect( humanizeLenAction, SIGNAL( triggered() ), m_editor, SLOT( humanizeNotesLen() ) );
+
+	QAction* strumNotesUpAction = 
+    new QAction(embed::getIconPixmap( "strumUp" ), tr( "Strum Selected by Q-value" ), this );
+	connect( strumNotesUpAction, SIGNAL( triggered() ), m_editor, SLOT( strumNotesUp() ) );
+
+    QAction* strumNotesDnAction = 
+    new QAction(embed::getIconPixmap( "strumDwn" ), tr( "Strum Selected by Q-value" ), this );
+	connect( strumNotesDnAction, SIGNAL( triggered() ), m_editor, SLOT( strumNotesDn() ) );
+
+
+//add physical buttons for each of the defined QActions, in this order


If I may, related to my comment above, I'd like to suggest new text for these:

'Humanize selected (velocity)'

'Humanize selected (start time)'

'Strum selected up'

'Strum selected down'

Adding to this, maybe renaming the pixmaps to humanizeVel, humanizePos and strumDown would make names more understandable.

SecondFlight · 2020-06-23T16:35:03Z

Thanks for the contribution! I like the ideas here. If I get time this evening I'll check out the build and do some bug testing too.

zonkmachine · 2020-06-23T17:23:04Z

src/gui/editors/PianoRoll.cpp

+                                       //This value is position, so it need to be qualified
+                                       //to real piano/roll values 
+  int strumSz = returnStrum(index); //size of note movement;
+  int strum = strumSz;//keep orr. value of selected strumming


orr.

Please type all of the word and just stop shorten words in general. I have asked you this before. It's hard to read what you write many times because of this.

zonkmachine · 2020-06-23T17:29:52Z

src/gui/editors/PianoRoll.cpp

+                                       //This value is position, so it need to be qualified
+                                       //to real piano/roll values
+  int strumSz = returnStrum(index); //size of note movement  
+  int strum = strumSz;//keep orr. value of selected strumming


strumSz

I suggest strumSize and also fix the comment (orr.) .

zonkmachine · 2020-06-23T17:43:29Z

src/gui/editors/PianoRoll.cpp

@@ -1573,7 +1609,7 @@ void PianoRoll::mousePressEvent(QMouseEvent * me )

 			x -= WHITE_KEY_WIDTH;

-			// get tick in which the user clicked
+	        // get tick in which the user clicked


Revert. Tabs. Please avoid formatting changes. This is a big enough PR anyway.

See above,

Also, please use tabs for formatting instead of spaces.

and all added lines in the PR have this issue.

src/gui/editors/PianoRoll.cpp

SecondFlight · 2020-06-25T01:03:25Z

I'm getting missing pixmaps when I install the lmmsbot build. Are these meant to be added in this PR, or would they need to be designed and added later?

SecondFlight · 2020-06-25T01:11:39Z

Just tested the strum and humanize functions. Very useful tools!

SecondFlight · 2020-06-25T01:15:31Z

Looks like notes can be humanized to before the start of a pattern. This makes them unreachable by the playhead, so they never play:

musikBear · 2020-06-25T20:33:46Z

@zonkmachine
Me:

I used the 'add-file-through-upload'.

You:

What's that? Are you uploading files via the GitHub interface?
Yes! The GitHub interface

I had the files exported from my virtual linux-machine through email...
absurdly i cant find a way to make my linux-mint connect to a common area for both OS. -nwm...
So then from my own master in correct sub-folder i used

Then i get a screen where i can browse to my new file, and add that to my master.
I had no idea, that it would be compared to current master, where my master seams to be at least one commit behind (as explained be @SecondFlight (thanks!)), and then it would result in my commit, apparently changing something i haven not touched at all 👎
Damn gitHub is more complicated than the code..
How can this predicament be avoided?
I can think of one way, where i do

Update my Master to current Master -> im on commit-equal-level
Open my code in editor
copy that code into MY current 'on commit-equal-level' Master
iMediately create PR

That would work for tiny additions like the 5 lines for a new envelope..

musikBear · 2020-06-25T20:54:22Z

@SecondFlight

I'm getting missing pixmaps

Yes, i hope @RebeccaDeField will draw something in correct style, she and umcaruje are the artists on 1.2.1 afaik, so correct no pixmaps, only tooltips

Looks like notes can be humanized to before the start of a pattern.

¤blush¤ thats a silly mistake -good find, i will need to check if the note is positioned < 0

@zonkmachine

proper functions

i felt the implementation with +/- was very clean and readable

Note to self: its was a HUGE mistake making a PR for more than ONE feature, these edits requests are making me confused, have i addressed all

anywitch, need to go back to the workshed ..

Just tested the strum and humanize functions. Very useful tools!

Thanks :p Lets see if i can re-introduce them in a proper way at all

RebeccaDeField · 2020-07-20T15:06:12Z

Yes, i hope @RebeccaDeField will draw something in correct style, she and umcaruje are the artists on 1.2.1 afaik, so correct no pixmaps, only tooltips

I'm okay with creating some icons if you need them, there seems to be a lot going on in the same PR here so just let me know what I can help with when you're ready to create separate PRs for each request.

Spekular · 2020-07-23T19:25:46Z

IMHO, this is unreviewable/unmergeable as is and should be split into 1 PR per feature.

New piano-like envelope for general instrument (Part of Making a good first-view impression #2092)
Humanize note position / velocity (speedbar-button) (Per-instrument Timing, Volume and Pitch Humanization/Randomization #1165)
Strum from top / bottom (speedbar-button) (Chord delay #2380)
Shorten / lengthen notes by 1/192 (key-combo to change note-length in piano-roll #3138)

@musikBear If you're concerned about losing your work in the process, you can export a patch file of these changes to be extra sure that you won't lose any work. Put that somewhere safe outside your git directory, push your local changes to this branch, and you definitely shouldn't lose any work. Then you can create a clean branch based on master, implement one of the above features in it, and open a new PR.

musikBear · 2020-07-24T01:51:02Z

@RebeccaDeField I made a bad descission when i bundled these features in one PR, just as @Spekular said. I will have to remake these as separate PR' with only one feature / PR.
The bundling thing was a result of only making a very simple modification of the default envelope, that was to nominal for a PR. Then i 'suddenly' had a handful of features.. All should not have been in same PR.
Lets wait with the artwork, until i have structured these PR' in a good way :)
@specular -would it be acceptable to just insert Comments around code-parts, and 'leave-it-out' in that way. Not even sure that would be the simplest way, but at least i would not have to delete code from the files, only change it to Comments with '/**/' ?

Spekular · 2020-07-24T06:29:51Z

@specular -would it be acceptable to just insert Comments around code-parts, and 'leave-it-out' in that way.

This would still leave a lot of changes in the diff (comparison) for reviewers to ignore, making reviews harder. I think you need to work from a clean slate.

With a standard git setup this shouldn't be too hard, the following should work:

git fetch --all
git checkout -b [name of new branch] origin/master

We can assist you on Discord with any git issues you run into, to avoid creating a bunch of github notifications by doing so in this thread.

musikBear · 2020-07-25T13:18:08Z

@Spekular ok i will look at it

zonkmachine · 2020-12-30T18:13:36Z

@musikBear Closing. Do you need help salvaging this code? Do your original local branch still work?

musikBear · 2020-12-31T13:16:22Z

Do you need help salvaging this code? Do your original local branch still work?

Nooo.. its 7 parameters ..lol wont even regard it as 'code', it the other stuff humanize and strum that is a bit of work, but i have old-school local copies of everything. I'm from a time where cardboard floppies was backup, that i still do, aside cardboard 🤣
Happy New-year!

musikBear added 11 commits April 2, 2020 12:30

Add files via upload

4536446

Changes to default envelope that removes clicks and pops for all instrument with envelopes

Add files via upload

4492e2c

2 Methods added: 1) void humanizeNotes(); Humanizes Selected Notes 2)…

5babcc8

… void strumNotes(); Strum selected notes with fixed value

Added prototypes for two new methods : void humanizeNotes(); & void s…

978ad43

…trumNotes();

Delete PianoRoll.cpp

b588a0a

removes erroneous added file

Delete PianoRoll.h

9605c36

removes erroneous added file

Update PianoRoll.cpp

6524ff7

Removed include for random and debug

Add files via upload

3bb9467

This update preserve all AFP-volume-envelopes and is fully backward compatible New objects from Sidebar| My-Instruments, will have a new more instrument-like std-envelope. It will default not be enabled.

Add files via upload

5d355c1

New prototypes for humanizing & strumming in piano-roll

Add files via upload

03f8318

Adding methods for * Humanizing selected note-position & Note-velocity * Strumming selected chords from Up & Down * Moving selected notes with + & - in 1/192 steps

SecondFlight requested changes Jun 23, 2020

View reviewed changes

zonkmachine requested changes Jun 23, 2020

View reviewed changes

This was referenced Jul 21, 2020

Reorganising LMMS, renaming editors and classes and everything in between (cont. #120) #5592

Closed

June2020m b #5538

Closed

zonkmachine mentioned this pull request Oct 8, 2020

Envelope for instruments #5709

Closed

allejok96 mentioned this pull request Dec 8, 2020

Icon changes in piano roll and song editor #5702

Closed

zonkmachine closed this Dec 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mB_prJune23_2020 #5544

mB_prJune23_2020 #5544

musikBear commented Jun 23, 2020

LmmsBot commented Jun 23, 2020 •

edited

Linux

Windows

macOS

SecondFlight left a comment •

edited

SecondFlight Jun 23, 2020 •

edited

SecondFlight Jun 23, 2020 •

edited

zonkmachine Jun 23, 2020 •

edited by SecondFlight

musikBear Jun 24, 2020

SecondFlight Jun 24, 2020

zonkmachine Jun 24, 2020

SecondFlight Jun 23, 2020

SecondFlight Jun 23, 2020

SecondFlight Jun 23, 2020

SecondFlight Jun 23, 2020 •

edited

ryuukumar Jul 22, 2020

SecondFlight commented Jun 23, 2020

zonkmachine Jun 23, 2020

musikBear Jun 25, 2020

zonkmachine Jun 23, 2020

zonkmachine Jun 23, 2020

SecondFlight Jun 23, 2020 •

edited

SecondFlight commented Jun 25, 2020

SecondFlight commented Jun 25, 2020

SecondFlight commented Jun 25, 2020

musikBear commented Jun 25, 2020

musikBear commented Jun 25, 2020

RebeccaDeField commented Jul 20, 2020

Spekular commented Jul 23, 2020

musikBear commented Jul 24, 2020

Spekular commented Jul 24, 2020

musikBear commented Jul 25, 2020

zonkmachine commented Dec 30, 2020

musikBear commented Dec 31, 2020

		const float lafI = 1.0f / qMax( minimumFrames, m_lfoAttackFrames );
		const float lafI = 1.0f / m_lfoAttackFrames;

mB_prJune23_2020 #5544

mB_prJune23_2020 #5544

Conversation

musikBear commented Jun 23, 2020

LmmsBot commented Jun 23, 2020 • edited

Linux

Windows

macOS

SecondFlight left a comment • edited

Choose a reason for hiding this comment

SecondFlight Jun 23, 2020 • edited

Choose a reason for hiding this comment

SecondFlight Jun 23, 2020 • edited

Choose a reason for hiding this comment

zonkmachine Jun 23, 2020 • edited by SecondFlight

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

SecondFlight Jun 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SecondFlight commented Jun 23, 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

SecondFlight Jun 23, 2020 • edited

Choose a reason for hiding this comment

SecondFlight commented Jun 25, 2020

SecondFlight commented Jun 25, 2020

SecondFlight commented Jun 25, 2020

musikBear commented Jun 25, 2020

musikBear commented Jun 25, 2020

RebeccaDeField commented Jul 20, 2020

Spekular commented Jul 23, 2020

musikBear commented Jul 24, 2020

Spekular commented Jul 24, 2020

musikBear commented Jul 25, 2020

zonkmachine commented Dec 30, 2020

musikBear commented Dec 31, 2020

LmmsBot commented Jun 23, 2020 •

edited

SecondFlight left a comment •

edited

SecondFlight Jun 23, 2020 •

edited

SecondFlight Jun 23, 2020 •

edited

zonkmachine Jun 23, 2020 •

edited by SecondFlight

SecondFlight Jun 23, 2020 •

edited

SecondFlight Jun 23, 2020 •

edited