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

Envelope for instruments #5709

Closed
wants to merge 14 commits into from
Closed

Envelope for instruments #5709

wants to merge 14 commits into from

Conversation

musikBear
Copy link

Learned the hard way.. I here only add one new feature. It is simply a new envelope, that will be available for all items dragged in from side-bar| Instruments.

It is default Off -A prerequest needed, because all our default Samples, uses a default AFP-i nstrument. That is unfortunate, but it cant be changed now.

Changes to default envelope that removes clicks and pops for all instrument with envelopes
… void strumNotes(); Strum selected notes with fixed value
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
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
Added few changes in default envelope, giving all new default instruments a piano-like envelope.
This envelope must be Off (0.0) because AFP uses default stup for its defined presets, unless a specific envelope has ben defined.
Non of the factory presets has a defined envelope. (this leads to clicks/pops in most of our default AFP 'presets' eg the raw sound-files).
static_cast<f_cnt_t>( frames_per_env_seg *
expKnobVal( m_attackModel.value() ) ) );
const f_cnt_t attack_frames = static_cast<f_cnt_t>( frames_per_env_seg *
expKnobVal( m_attackModel.value() ) );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've pushed something like this before and the discussion here is still valid:
#5544 (comment)

Copy link
Author

@musikBear musikBear Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not added by me
In this PR only parameters in the envelope definition is changes, nothing else
Why git marked this as added, i have no idea.
I use Notepad++ as editor.
Content of code:
image

Oscar, should i take current Masters complete EnvelopeAndLfoParameters.cpp -eg the current fully updated code, and change those 8 real values in that file, then add that to my own Master, create a new PR, and expect the oddness around line 410 to be gone. Never even added a white-space beyond line 102..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are working from your master and your master branch has your stuff on top of it. It isn't 'clean'. Also it looks like you are still working from the github web interface and not pushing your changes from your computer directly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it looks like you are still working from the github web interface and not pushing your changes from your computer directly.

That is correct. I add the files on the git-UI.
Why would that be a problem?
I have washed all part of the repo away (its archived as of now, but i will propl. delete later)
I plan to fork current master, and then to add the 7 lines of edited code
That is impossible to botch.. isent it..?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would that be a problem?

This is how you do it: https://github.com/LMMS/lmms/wiki/Contributing-Step-by-Step
Especially the part on submitting a patch: https://github.com/LMMS/lmms/wiki/Submitting-a-patch

Have you set up your remotes? (origin, upstrem etc.)
Run this command anywhere in your lmms code tree:
git remote --verbose

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take this up with gitHub, if they are willing to - At least i would get an understanding for why this is not working.

Please don't bump GitHub about this, I'm sure they have better things to do.

Edit-file > Commit
Should lead to any issues at all. The changed code should appear as '+' in the compare, and the previous as '-'.
Absolutely nothing else should be marked.

I'm pretty sure the changes you are about to commit are visible somewhere and you've just missed it.

I think the problems you are seeing here is threefold.

  1. You are working from the master branch. Always make a separate branch for the new changes and keep the lmms branches (master, stable-1.2 etc.) clean.
  2. Your master branch isn't the same as lmms/master.
  3. You don't make commits from a branch but add the files you are working on via the online editor. This makes it hard (at least for me) to follow what your doing. If you

Points 1, 2 and 3 above are all solved by following the instructions on our wiki page. Maybe you have a method that actually is legitimate but the PR's you have made so far suggest that this isn't the case. I will not be able to assist you here any further unfortunately as you don't follow a method that I have any deeper knowledge in. Sorry!

Copy link
Author

@musikBear musikBear Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will not be able to assist you here any further unfortunately as you don't follow a method that I have any deeper knowledge in

Fully understandable -Thanks so far.

Copy link
Member

@tresf tresf Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@musikBear as @zonkmachine has explained, you should create a new branch and do each PR from there.

Unfortunately when you edit master as you've done you start cloning your changes to the new branches too, so you need to reset the branch your working on manually before making changes. I use the command git reset --hard 85e0574 (or a similar commit).

In the case of this PR, you can hard-reset using the above command and then bring back the changes you want manually. You'll want to force push AFTER you bring forward your changes.

Ultimately -- to avoid this nonsense -- you shouldn't ever edit master, just fast forward it once in a while. This is an unwritten rule that all "forks" follow.

If you're OK with putting master back to where it should be, you can hard reset that too. I'd be happy to help you with this if needed.

Copy link
Author

@musikBear musikBear Oct 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

help you with this if needed.

I have like 10 jokes pending.. Guess "If the pope is catholic, its needed" would do.
I will definitively need help, but i do have a plan
I have already made my current repo into a archived-repo
That should make it possible to start from scatch(?)
I therefore plan to make a brand new copy of newest Master as my new own Master.
I have gitDesktop installed.
I ought to be able to

  • take one single file, and commit that to my new-master.
  • create a PR from that new Master, that now should include one new file
    Ask for review of that change.

Does that sound as a functional plan?
I dont understand why there are options for adding new files directly in github.com, and then commit these changes, again with the tools on github.com, where there simply is a button labelled Commit, when it messes everything up.
I have asked on gitHub forum, but the question remains unanswered.
First i need to make a new Own-Master based on current. I am reading up on that, atm :)
I will come back to you, if i get stuck I have never used gitDesktop, so i have a steep climb, i am afraid -Much Appreciated! 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont understand why there are options for adding new files directly in github.com, and then commit these changes, again with the tools on github.com, where there simply is a button labelled Commit, when it messes everything up.

GitHub's website functionality is a convenience and isn't really intended as a full replacement to a code editor (at least not yet, this will likely get better in the future). But that's just fine, the website commits can be squashed like any others. That's not the problem. The problem is when you make a new feature branch, it copies the stuff you did to master. So leave master alone. Easy right? No, we all edit master because it's the default branch we clone, so this problem is very common. It's not unique to git nor unique to GitHub, but I'll digress. Leave master alone.

create a PR from that new Master, that now should include one new file Ask for review of that change.

No, don't edit master. Reset it to match lmms/lmms and leave it that way. Only fast forward it once in a while. Do your changes on the branch by selecting it from GitHub's dropdown menu (or better, switching your branch locally and using your PC to edit the changes). <3

help you with this if needed.

I have like 10 jokes pending.. Guess "If the pope is catholic, its needed" would do.

Sorry, I don't understand the joke, but my offer still stands. The proverb I was raised with...

"Give a man a fish and you feed him for a day. Teach a man to fish and you feed him for a lifetime."

Let's 🎣

@zonkmachine
Copy link
Member

What happened here is that the original commits were made on stable-1.2 . That and the work process described above. I'll try and see if it can be moved to master but in this case I think the best idea is for @musikBear to manually copy the strings over from stable-1.2 to master. I'll close this and the other one for now. It's better to make a new PR from scratch.

And next time, keep them:

  • Small, one feature per commit.
  • No format changes.

That way we can more easily review it. I wouldn't try and switch the branch manually to stable-1.2 this time. It's no use as it's going to master anyway.

@zonkmachine zonkmachine closed this Dec 3, 2020
@musikBear
Copy link
Author

Thanks -Appreciated!

And next time, keep them:
Small, one feature per commit.

Oh... ye, If only i...

No format changes.

That one was odd. I cant figure out why it happened, i only used TAB in those for humanization ..idk 😖

@musikBear musikBear deleted the Envelope-for-Instruments branch January 22, 2022 16:48
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.

None yet

3 participants