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

Step Recording feature #4544

Merged
merged 11 commits into from
Feb 9, 2019
Merged

Conversation

Mister-Lemon
Copy link
Contributor

@Mister-Lemon Mister-Lemon commented Aug 19, 2018

Step Recording feature adds a new type of recording in which the user record in steps resolution.
(Addresses #1421)
e.g.:
step_recording_wip2

**PR is fully functional! **

Behaviour description:

  • Toggle step-recording mode using the dedicated icon

  • (This mode is mutually exclusive with other recoding modes (record/record accompany))

  • Step-Recording while song is playing is allowed (and fun! :) )

  • When start recording, the start recoding-position will be set where the timeline curser points (quantized backwards using PianoRoll's current quantization)

  • Step length is determined by PianoRoll's current note-length (can be changed dynamically during step-recording)

  • The record-position can be moved forward/backward using right/left keys
    step_recording_wip4

  • When notes are pressed on keyboard/midi-device, they will be added temporarily ("recorded") with a length of a step. while still pressed, user can adjust the length by steps resolution using the arrow keys (e.g. moving right once will make the note's length 2-steps, another right press will make the length 3-steps etc.)
    step_recording_wip5

  • When all pressed-keys are released, the actual recording happen and the notes are added.

  • If the user press multiple notes, and release some of them for some time which indicates it is intentional i.e. he didn't want to do a full release to record the step but rather just change what will be recorded (I set the "intentional release threshold" to 70 milliseconds) - these note will be removed from current step-recording. e.g.
    step_recording_wip3

  • Added notes are not quantized, making the addition simpler and WYSIWYG

  • Similiarly to adding notes using mouse clicks, an undo-checkpoint is added per added step and not for the whole recording as in other record modes.

Still missing:

  • Icons (I made temporary ones, waiting for suggestions...)

Done:

  • Focus on start position when start recording
  • Auto-scroll when record position becomes off-screen
  • On start step-recording : show message to user about using right and left arrows to move recording position

Let me know what you think,
Thanks!

update1:
I modified the code so that editing is allowed during step-recording.
now, the currently recorded step is not subjected to modifications. e.g. deleting using mouse right-click:
step_recording_wip6

update2:

  • Now there is no limition with adding new notes using mouse while step-recording.
    In such case, the addition will work but the note will not play during the addition.
    This behaviour is aligned with what happens when user add note during other recording modes
  • I added usage hint that will show on the bottom of the main window when toggling step recording on:
    hint

update3:
Feature is fully functional. I added the last part - auto scroll to current step-recording cursor position.
(known limitation: only works if position is within pattern's current length. this is the most common use case so I think it is fine as-is. working around this limitation would require (ugly) changes in auto-scroll implementation and does not seems justified for such corner case)

@LmmsBot
Copy link

LmmsBot commented Aug 20, 2018

Downloads for this pull request

Generated by the LMMS pull requests bot.

@Mister-Lemon
Copy link
Contributor Author

Hi,
please wait with code reviews for now(in case someone planned to do it) as I am refactoring large portion of StepRecorder.cpp and plan to upload the changes soon.

My aim for the upcoming update is to remove the editing restriction while step-recording.

Feedback regarding how the feature behaves and also gui suggestions(icon, piano-roll appearance etc.) is still welcome :)

if (AttachConsole(ATTACH_PARENT_PROCESS) || AllocConsole())
{
freopen("CONOUT$", "w", stdout);
freopen("CONOUT$", "w", stderr);
Copy link
Member

Choose a reason for hiding this comment

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

Though it should not be a part of this PR, AttachConsole + freopen workaround may be useful. 👍

Copy link
Contributor Author

@Mister-Lemon Mister-Lemon Aug 24, 2018

Choose a reason for hiding this comment

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

Hi @PhysSong ,

This code is under ifdef and does not enter production executable. I added it in order to be able to debug easier as I could not find how to get prints out.
To enable it, I uncomment //#define DEBUG_STEP_RECORDER and build.

Since it does not affect production executable, is there an issue with keeping it?
what is the right way of adding it to make future debugging easier?

Copy link
Member

Choose a reason for hiding this comment

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

You can wrap debug prints with LMMS_DEBUG preprocessor definition which is defined only in debug builds. I also suggest using Qt's functions instead of plain printf.
For the Windows-specific console code, I'd say it should be in a separate pull request. It's totally unrelated to this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. how do you use LMMS_DEBUG ? (where it is defined? is it triggered by some build flag?)
  2. I tried to use QT's print functions before using printf and it did not work for some reason (did not output to console. I couldn't figure why)
  3. I am not sure I understand, are you suggesting I should have two onging PRs on the same file?
    how can see debug prints in the meanwhile? I need something I can work with and debug the component I work on... can you please provide some alternative?

Copy link
Member

Choose a reason for hiding this comment

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

how do you use LMMS_DEBUG? (where it is defined? is it triggered by some build flag?)

It's defined in the root CMakeLists.txt. The macro is defined in debug builds.

I tried to use QT's print functions before using printf and it did not work

Could you let me know your OS, terminal and Qt version?

I am not sure I understand, are you suggesting I should have two onging PRs on the same file?

I think you should move the console code to somewhere else and open a new PR.

Copy link
Contributor Author

@Mister-Lemon Mister-Lemon Aug 24, 2018

Choose a reason for hiding this comment

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

It's defined in the root CMakeLists.txt. The macro is defined in debug builds.

👍

Could you let me know your OS, terminal and Qt version?

Win10 and QT5. (is terminal question revelant to win?)

I think you should move the console code to somewhere else and open a new PR.

I will move it, but probably won't open PR for it; I'll keep it locally.

@PhysSong
Copy link
Member

Please make sure you've used only tabs for indentation when you've implemented everything! 😃

@Mister-Lemon
Copy link
Contributor Author

Hi, I would like to get feedback on the following:

  1. usage hint:
    hint

  2. Icons: On: image Off: image

@Mister-Lemon
Copy link
Contributor Author

Feature is now fully functional! :)

@PhysSong
Copy link
Member

@Mister-Lemon Thanks for this work! I'll test and review this in a few days.

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

I found some formatting issues that you should/may address.
I haven't tested the feature much, but I'll do some more tests soon.

// set new data
m_pattern = newPattern;
m_currentPosition = 0;
m_currentNote = NULL;
m_startKey = INITIAL_START_KEY;

if( ! hasValidPattern() )
if(hasValidPattern())
Copy link
Member

Choose a reason for hiding this comment

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

We used to use if-return pattern to reduce indentation. I think it'll be better if you revert changes from here and add m_stepRecorder.setCurrentPattern(newPattern); before this if statement if the change does not break anything.

StepRecorderWidget::StepRecorderWidget(
QWidget * parent,
const int ppt,
const int margin_top,
Copy link
Member

Choose a reason for hiding this comment

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

It's not an explicit convention, but we prefer camelCase to snake_case.


void StepRecorderWidget::showHint()
{
TextFloat::displayMessage( tr( "Hint" ),
Copy link
Member

Choose a reason for hiding this comment

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

According to our coding convention, you shouldn't put whitespaces in parentheses in new files. You can put them in old files for consistency though.

@@ -261,7 +271,7 @@ protected slots:
void autoScroll(const MidiTime & t );

MidiTime newNoteLen() const;

Copy link
Member

Choose a reason for hiding this comment

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

Please remove trailing whitespaces which you've added.

* Boston, MA 02110-1301 USA.
*
*/
#ifndef _STEP_RECOREDER_WIDGET
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove _ at the beginning.

int nextTimout = INT_MAX;
bool notesRemoved = false;

QMutableVectorIterator<StepNote*> itr(m_curStepNotes);
Copy link
Member

Choose a reason for hiding this comment

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

Remove a space after a tab.

public:
StepRecorderWidget(
QWidget * parent,
const int _ppt,
Copy link
Member

Choose a reason for hiding this comment

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

In function parameters as well.


QColor curStepNoteColor() const
{
return QColor(245,3,139); // radiant pink
Copy link
Member

Choose a reason for hiding this comment

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

I think we can move this to stylesheets later.

m_marginLeft(marginLeft),
m_marginRight(marginRight)
{
const QColor baseColor = QColor(255, 0, 0);// QColor(204, 163, 0); // Orange
Copy link
Member

Choose a reason for hiding this comment

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

Here as well.

include/StepRecorder.h Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

PhysSong commented Sep 3, 2018

QObject::connect: No such slot PianoRollWindow::patternRenamed() in /home/travis/build/LMMS/lmms/src/gui/editors/PianoRoll.cpp:4382
QObject::connect: No such slot PianoRollWindow::patternRenamed() in /home/travis/build/LMMS/lmms/src/gui/editors/PianoRoll.cpp:4383

You'll also need to change remaining patternRenamed to updateAfterPatternChange.

@PhysSong
Copy link
Member

I found that I can step-record patterns while playing it. Is it intended?

@Mister-Lemon
Copy link
Contributor Author

Yes, see bullet 3 in the description above

@PhysSong
Copy link
Member

I missed that, thanks!
I also found some bugs.

Bug 1

  • Start step recording and hold a key
  • Press the right arrow key and then press another key
  • Release two keys simultaneously

Then two notes will have different lengths. I'm not sure it's a bug, but it looks like a bug to me.

Bug 2

  • Hold a key and go back a few steps
  • Press new keys and go forward

It will behave somewhat weird.

@Mister-Lemon
Copy link
Contributor Author

Thanks for testing! I will look into it

@Mister-Lemon
Copy link
Contributor Author

@PhysSong , thanks for reviewing and testing the feature!
I fixed the bugs you found, can you please verify the fix?

@PhysSong
Copy link
Member

Will do that in a few days. :)

@Mister-Lemon
Copy link
Contributor Author

I am dropping support for this PR.
If anyone wants to take ownership, he is welcome.

@Reflexe
Copy link
Member

Reflexe commented Oct 11, 2018

@Mister-Lemon To help us improve, can I ask why have you dropped this PR?

@PhysSong
Copy link
Member

I am dropping support for this PR.

That's okay. If no one takes this PR, I will.
BTW, why did you decide like this?

@Mister-Lemon
Copy link
Contributor Author

Hi,
It just became a burden having it in 'open' state for so long.
Thank you @PhysSong and good luck!

@PhysSong
Copy link
Member

Okay. Hope you'll come with new works later. :)

@qnebra
Copy link

qnebra commented Dec 1, 2018

Hi,
It just became a burden having it in 'open' state for so long.

Normal in this repository. Good PRs waiting endlessly, their devs lose all hope and enthusiasm and move on to other things.

includes all basic functionallty.
Added two new classes: StepRecorder(logic) StepRecorderWidget(gui).
still missing: Ignoring mouse edit events during step recording
…ows editing while is step-recording mode)

This change allows editing while is step-recording mode.
The issue with editing was with manipulating just the current step being recorded (while pressing the notes), which might caused intervation to StepRecorder logic.
The resolution in this commit is to keep the current step notes in a seperate list inside StepRecorder (instead inside Pattern).
This way, these sepcific notes cannot be modified using the regular actions (e.g. right click to delete).
Due to this change, PianoRoll needs to paint these notes expliclty besides the other notes in Pattern. (which also turned nicely since it made it easy to paint them in different color.
This change also include various refactoring inside StepRecorder.cpp
…. add usage-hint when toggling step-recoprding on

1. prevent playing note when added using mouse while step-recording
This change is aligned to current behaviour during regular recording

2. add usage-hint when toggling step-recoprding on.
will show "Move recording curser using <Left/Right> arrows" on bottom of main window

(3. change space indentation into tabs)
Auto-scroll to step-recording cursor position.
(know limitation: only works if position is within pattern's current length (i.e. works fine for the usual use case))
Mister-Lemon and others added 4 commits February 2, 2019 18:43
…o updateAfterPatternChange

In a former commit, the slot function "patternRenamed " was renamed to "updateAfterPatternChange" but two places in code were not changed accordingly by mistake. This issue now fixed.
Also, removed trailing-spaces from files.
…fic cases)

1. length of new notes was wrong when current step was already in a size of multiply of steps.

2. recorded notes start-position was not updated correctly after moving cursor backwards
@zonkmachine
Copy link
Member

I fixed the bugs you found, can you please verify the fix?

@PhysSong
Bug 1. I can verify both the issue and the fix. 👍
Bug 2. I don't see the issue in the first place. By 'going back a few steps' I assume you mean by pressing the left arrow? Nothing strikes me as odd here. I keep on testing though.

This PR has conflicts since #4575 was merged. I've rebased it against master (b68c5ee), fixed the conflict, and intend to push this together with some whitespace fixes.

@zonkmachine zonkmachine force-pushed the feature/step-recording branch from d4fee61 to d035e43 Compare February 2, 2019 22:06
@zonkmachine
Copy link
Member

  • When start recording, the start recoding-position will be set where the timeline curser points (quantized backwards using PianoRoll's current quantization)

Which can be a bit confusing because turning step recording on while playing is allowed and the start recording position is then a bit random unless you're dead on the beat. Maybe in the case where the pattern/song is playing, the recording position should be relative to the start of the pattern. Position 0.

Copy link
Member

@zonkmachine zonkmachine left a comment

Choose a reason for hiding this comment

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

Tested. Function wise this is good to merge. I haven't looked deeply into the code though. There are issues mentioned in this PR but none of them are big enough to hold back the PR.

@zonkmachine
Copy link
Member

zonkmachine commented Feb 3, 2019

Maybe in the case where the pattern/song is playing, the recording position should be relative to the start of the pattern. Position 0.

if(hasValidPattern())
{
m_stepRecorder.start(Engine::getSong()->getPlayPos(Song::Mode_PlayPattern), newNoteLen());
}

Tested. Seem to work.

	if(hasValidPattern())
	{
		if(Engine::getSong()->isPlaying())
		{
			m_stepRecorder.start(0, newNoteLen());
		}
		else
		{
			m_stepRecorder.start(
				Engine::getSong()->getPlayPos(
					Song::Mode_PlayPattern), newNoteLen());
		}
	}

@zonkmachine
Copy link
Member

^OK. Pushed that. The original PR is backed up by the way.

@CaBachhav
Copy link

A very very important function is to be added to this Step sequencer , if it is added the LMMS will become better than FLstudio.that is in old school mod trackers and latest OPENMPT tracker , number of "steps seek" to be assigned before note is put. Ex. If we put 4 step or N step , the step sequencer cursur jumps forward to 4 steps or N steps. this N can be anything from 1 to 32 .

@zonkmachine
Copy link
Member

zonkmachine commented Feb 6, 2019

I suggest we merge (my fix included) unless someone else wants to test/review this some more.

src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
src/core/StepRecorder.cpp Outdated Show resolved Hide resolved
@CaBachhav
Copy link

Anybody working on step Curser jump Function ? Which is very useful in old school Amiga type Mod Trackers, fast tracker etc. Denoted as ADD function.

@zonkmachine
Copy link
Member

Anybody working on step Curser jump Function ?

I don't think so. Please open an issue for this if you like:
https://github.com/LMMS/lmms/issues/new

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
(Addresses LMMS#1421)

**Behaviour description:**

* Toggle step-recording mode using the dedicated icon.
* This mode is mutually exclusive with other recoding modes (record/record
  accompany).
* Step-Recording while song is playing is allowed (and fun! :) ).
* When start recording, the start recording-position will be set where the
  timeline curser points (quantized backwards using PianoRoll's current
  quantization). If step-recording is started while the pattern is playing the
  start recording-position is set to the beginning of the pattern.
* Step length is determined by the Piano Roll's current note-length (can be
  changed dynamically during step-recording).
* The record-position can be moved forward/backward using the right/left keys.
* When notes are pressed on keyboard/midi-device, they will be added
  temporarily ("recorded") with a length of a step. while still pressed, user
  can adjust the length by steps resolution using the arrow keys (e.g. moving
  right once will make the note's length 2-steps, another right press will make
  the length 3-steps etc.).
* When all pressed-keys are released, the actual recording happen and the
  notes are added.
* If the user press multiple notes, and release some of them for some time
  which indicates it is intentional i.e. he didn't want to do a full release
  to record the step but rather just change what will be recorded (I set the
  "intentional release threshold" to 70 milliseconds) - these note will be
  removed from current step-recording. e.g.
* Added notes are not quantized, making the addition simpler and WYSIWYG
* Similiarly to adding notes using mouse clicks, an undo-checkpoint is added
  per added step and not for the whole recording as in other record modes.
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.

7 participants