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

Global Groove Quantization #4232

Draft
wants to merge 26 commits into
base: master
Choose a base branch
from
Draft

Global Groove Quantization #4232

wants to merge 26 commits into from

Conversation

tresf
Copy link
Member

@tresf tresf commented Mar 9, 2018

groove
misc

This supersedes #3296, so there may be some contextual conversations there of importance.

This is an effort coded by @teknopaul but due to merge conflicts has been taken over by @Sawuare / @PhysSong to make it into the 1.3.0 milestone.

This pull request illustrates "groove quantizing" the same "swing" feature that Hydrogen drum machine has.
Currently, there is one global swing amount that affects all instruments in a song. The ability to disable groove per track would be useful.

TODO:

  • Complete the TODOs and FIXMEs in the code.
  • Move enable/disable groove per instrument track from the TrackOperationsWidget to the InstrumentMiscView.
  • Move groove near tempo and time signature controls.
  • Use an AutomatableSlider instead of a Knob for changing groove amount.
  • Style the groove slider like the master volume and pitch sliders.
  • Refactor.
  • Fix coding conventions.
  • Update the factory templates.
  • Move groove settings out of the tuning tab

Closes #1091.

Add initial support for "Groove quantizing".  This is a squash of all work by @teknopaul, @tresf, @Sawuare to make it mergabe against master.
@tresf tresf added this to the 1.3.0 milestone Mar 9, 2018
@tresf tresf changed the base branch from stable-1.2 to master March 9, 2018 16:55
src/core/Groove.cpp Outdated Show resolved Hide resolved
public:
GrooveExperiments(QObject *parent=0 );

virtual ~GrooveExperiments();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of defining ~GrooveExperiments with an empty body in the .cpp file, you can just say virtual ~GrooveExperiments() = default here and omit the body.


}

int GrooveExperiments::amount()
Copy link
Member

Choose a reason for hiding this comment

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

This method should be const?


HalfSwing::HalfSwing(QObject * _parent) :
QObject( _parent ),
Groove()
Copy link
Member

Choose a reason for hiding this comment

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

If you aren't passing parameters into the Groove constructor, you can omit this Groove() bit - Groove will be default-initialized.

@@ -149,6 +151,10 @@ InstrumentTrack::~InstrumentTrack()

// now we're save deleting the instrument
if( m_instrument ) delete m_instrument;

if (m_noGroove != NULL) {
delete m_noGroove;
Copy link
Member

Choose a reason for hiding this comment

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

FWIW it's valid to delete a NULL pointer (it just doesn't do anything). Putting this behind a conditional is superfluous.

@PhysSong
Copy link
Member

I'm going to do some refactoring to reduce code duplication. @Sawuare Is it fine for you to push my work once done?

@husamalhomsi
Copy link
Member

@PhysSong Absolutely. :)

PhysSong and others added 4 commits March 28, 2018 09:44
Put Half after Hydrogen because Half is based on Hydrogen.
And some improvements
@husamalhomsi
Copy link
Member

The groove slider code is duplicated in these 3 groove types: Hydrogen swing, Half swing, and Groove experiments.
I tried to refactor the code into Groove view to make the groove types mentioned above use the same slider (so that changing the groove type does not reset the slider), but failed. @PhysSong, are you interested in refactoring it?

@PhysSong
Copy link
Member

I want to consolidate them, too. I'm also going to refactor whole widgets' code. And next, I'm going to look into core functions.

@PhysSong
Copy link
Member

I think we should change class hierarchy a lot to make the code better.
GrooveView should be the base class for views for individual grooves, not the view in the song editor. It will reduce code duplication and make it easy to manage views for grooves.
I'm going to create new classes, GrooveManager and GrooveManagerView. The new view will play a role which was done by GrooveView.
I think it might be better to allow creating grooves as external plugins. To do this, I'll remove uses of hard-coded list in GrooveFactory::create to allow adding new grooves flexibly.

If anyone have some ideas, please let me know.

Hussam Eddin Alhomsi and others added 3 commits April 28, 2018 18:28
@husamalhomsi
Copy link
Member

@PhysSong I think it would be best to refactor the code first, so you go ahead.

@PhysSong PhysSong self-assigned this Nov 7, 2019
@LmmsBot
Copy link

LmmsBot commented Nov 19, 2019

🤖 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

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/d6b42ac6-2ba6-4dc9-be34-6f00758f2cce/artifacts/0/lmms-1.3.0-alpha.1.215+g193c30f88-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17260?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://output.circle-artifacts.com/output/job/264c4a6f-2ee8-45e6-a2f0-7710b65f2443/artifacts/0/lmms-1.3.0-alpha.1.215+g193c30f88-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17256?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/4872c59c-a8cd-435f-ba2e-db49d4a17672/artifacts/0/lmms-1.3.0-alpha.1.215+g193c30f88-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17257?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/cfsbhc89kdru5qp2/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43900197"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/t8tj3glwd3qw4jgy/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43900197"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/yv5sgni4gbarlvaa/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43900198"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/614xit5o0jckhvar/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/43900198"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/5cdb317b-2331-4982-af50-604b7f3f3605/artifacts/0/lmms-1.3.0-alpha.1.215+g193c30f88-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/17258?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "193c30f888a1db2cad7f699eb37a00d4e416234d"}

int posInEigth = -1;
if (posInBeat >= 36 && posInBeat < 48)
{
// third quarter
Copy link
Member

Choose a reason for hiding this comment

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

Is this 0-based or 1-based? The comments look inconsistent.

Copy link
Member

Choose a reason for hiding this comment

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

This comment says the 3rd quarter starts at tick 36, but another comment says the 2nd quarter starts at tick 12...

@PhysSong
Copy link
Member

@Sawuare Please feel free to add commits before I do, I need some time to design classes and the hierachy.

@tresf tresf marked this pull request as draft January 13, 2021 20:42
@Spekular Spekular modified the milestones: 1.3, 1.3+ Mar 12, 2021
@PhysSong PhysSong added rework required Pull requests which must be reworked to make it able to merge or review and removed in progress rework required Pull requests which must be reworked to make it able to merge or review labels Feb 5, 2022
@Gps2010
Copy link

Gps2010 commented Mar 5, 2024

Hi guys
Is this still work in progress, or should I abandon all hope? :)

@husamalhomsi husamalhomsi removed their assignment Mar 6, 2024
@zonkmachine
Copy link
Member

Is this still work in progress, or should I abandon all hope? :)

Both I believe...

@PhysSong
Copy link
Member

In the meantime, I resolved merge conflicts. I hope I can get back to this one next month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swing function.
10 participants