Sample editor instant feedback #1314

charbeljc · 2021-07-05T07:42:58Z

Update target wave display when loop, rubberband or envelope are edited.

Also should close #811
Also should close #1304

There are still strange things with rubberband, but we should be close to something usable.

theGreatWhiteShark · 2021-07-07T18:14:17Z

src/core/Basics/Sample.cpp

@@ -51,18 +50,18 @@ static RubberBand::RubberBandStretcher::Options compute_rubberband_options( cons


 /* EnvelopePoint */
-EnvelopePoint::EnvelopePoint() : Object( EnvelopePoint::__class_name ), frame( 0 ), value( 0 ) 


How come you removed the H2Core::Object in here? It's used for debugging and shouldn't add too much weight.

How come you removed the H2Core::Object in here? It's used for debugging and shouldn't add too much weight.

Hum, I was maybe a bit to quick with this. I think EnvelopePoint should go away in favor of a proper Envelope class.
Besides, I have a pending patch which removes the H2Core::Object's overhead...

My point was to get rid of the unique_ptr around EnvelopePoint in envelope, so I can access them from python. I don't remember the details right now, but pybind11 caused me some troubles with those std::shared_ptr<std::unique_ptr> ...

ihmo, EnvelopePoints should be plain dummy structs with copy semantics, and let the (to be written) Envelope let deal with them.

Hi @theGreatWhiteShark I just restored EnvelopePoint inheritance of H2Core::Object.
Regards

I think EnvelopePoint should go away in favor of a proper Envelope class.
Besides, I have a pending patch which removes the H2Core::Object's overhead...

Could you split off these changes and make a separate PR? Such fundamental changes in the code base require the consent of at least a majority of all maintainers. This will happen much faster if it's a concise one.

One thing to think about in advance (and I haven't done yet, so, there is a chance none of this makes sense): The envelope in the sample editor is quite similar to the automation path. Maybe there could be a common basis.

I think EnvelopePoint should go away in favor of a proper Envelope class.
Besides, I have a pending patch which removes the H2Core::Object's overhead...

Could you split off these changes and make a separate PR? Such fundamental changes in the code base require the consent of at least a majority of all maintainers. This will happen much faster if it's a concise one.

Yes, I will do a separate PR on this.

One thing to think about in advance (and I haven't done yet, so, there is a chance none of this makes sense): The envelope in the sample editor is quite similar to the automation path. Maybe there could be a common basis.

I will have a look at those automation path. If we could factor out some code, it would be nice.

Please see #1318 and #1319 (this one is extracted from #1313)

Should close hydrogen-music#811 and hydrogen-music#1304

theGreatWhiteShark

I like this signal-based version for communicating changes in the envelope or sliders more.

However, some parts are now broken:

Looping doesn't seem to work anymore and if the loop count is set to a value higher than 0 there are visual artifacts in the DetailWaveDisplay
Pitch settings using Rubberband does not seem to take any effect either.
Previously, when hitting play the white slider indicating current playback position was visible in the TargetWaveDisplay too
(unrelated to this PR but I just noticed it) when increase the loop count to very large values, like 300, Hydrogen freezes. We probably should add some sanity checks in there

theGreatWhiteShark · 2021-07-19T18:26:09Z

src/gui/src/SampleEditor/SampleEditor.cpp

-
+
+	if (nInstr == -1) {
+		pInstrument = nullptr;


How about replacing this line by an ERRORLOG and a return statement? If for any reason nInstr == -1 is true, there will be a segfault later on.

theGreatWhiteShark · 2021-07-19T18:28:10Z

src/gui/src/SampleEditor/SampleEditor.cpp

@@ -197,22 +253,24 @@ void SampleEditor::getAllFrameInfos()
 	if ( pLayer ) {
 		pSample = pLayer->get_sample();


same here. An ERRORLOG and return might prevent a crash in some edge-cases.

theGreatWhiteShark · 2021-07-19T18:30:23Z

src/gui/src/SampleEditor/SampleEditor.cpp

+		pInstrument = pInstrList->get( nInstr );
+		//INFOLOG( "new instr: " + pInstrument->m_sName );
+	}
+
 	assert( pInstrument );

 	auto pCompo = pInstrument->get_component(0);


I just now realize that using the SampleEditor only the first InstrumentComponent can be edited. Well, since nobody seems to use this feature anyway, nobody complained so far (and I don't think it's worth adding support for it until someone asks for it)

Okay, while only the first component is painted and used to set the values of all the spinboxes, the previous version of on_PlayPushButton_clicked did respect the selected component. So, it was not displayed properly but you could listen to it. This does not feel right.

theGreatWhiteShark · 2021-07-19T18:31:46Z

src/gui/src/SampleEditor/SampleEditor.cpp

 	assert( pSample );

 //this values are needed if we restore a sample from disk if a new song with sample changes will load
 	m_bSampleIsModified = pSample->get_is_modified();
 	m_nSamplerate = pSample->get_sample_rate();
 	__loops = pSample->get_loops();
 	__rubberband = pSample->get_rubberband();
+	qWarning() << "gafi rb use " << __rubberband.use


This debugging message probably did slipped through

theGreatWhiteShark · 2021-07-19T18:39:14Z

src/gui/src/SampleEditor/SampleEditor.cpp

 			pitchdoubleSpinBox->setValue( 0.0 );
 		}

-		if( __rubberband.divider == 1.0/64.0) {
+		setSamplelengthFrames();
+		qWarning() << "currentRatio: " << computeCurrentRatio();


this one slipped through too

theGreatWhiteShark · 2021-07-19T18:39:43Z

src/gui/src/SampleEditor/SampleEditor.cpp

+		setSamplelengthFrames();
+		qWarning() << "currentRatio: " << computeCurrentRatio();
+		if ( !__rubberband.use || (std::abs(computeCurrentRatio() - 1.0) < 0.0001) ) {
+			qWarning() << "Setting";


this one slipped through too

theGreatWhiteShark · 2021-07-19T19:05:53Z

src/gui/src/SampleEditor/SampleEditor.cpp

 	m_pTargetSampleView->move( 1, 1 );
+	m_pTargetSampleView->updateDisplay( sample, 1.0 );


AFAICS this function is only called in the constructor. Right afterwards getAllFrameInfo() is called which too calls m_pTargetSampleView->updateDisplay but with an InstrumentLayer. Next, doneEditing() is called which, again, calls m_pTargetSampleView->updateDisplay. This feels like the first two (including the line I comment on) are redundant.

theGreatWhiteShark · 2021-07-20T14:16:01Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.h

@@ -73,11 +66,12 @@ class MainSampleWaveDisplay : public QWidget, public H2Core::Object
 		void testPosition( QMouseEvent *ev );
 		void chooseSlider( QMouseEvent *ev );
 		void mouseUpdateDone();
-
+
+		std::shared_ptr<H2Core::Sample> m_pEditedSample;


What's the purpose of this member variable? It's never used.

theGreatWhiteShark · 2021-07-20T14:19:27Z

src/gui/src/SampleEditor/SampleEditor.cpp

+			m_pTargetSampleView->setEditMode( SampleEditor::EnvelopeType::VelocityEnvelope );
+			break;
+		case 1 ://
+			if ( m_pTargetSampleView ) {


This sanity check and the one below should already covered by the one in the beginning of the function

theGreatWhiteShark · 2021-07-20T14:22:13Z

src/gui/src/SampleEditor/TargetWaveDisplay.cpp

+
+void TargetWaveDisplay::updateDisplay( std::shared_ptr<H2Core::Sample> sample, double gain )
+{
+	// qWarning() << "TargetWaveDisplay::updateDisplay: sample:" << sample;


The original version of updateDisplay also included a check whether pLayer->get_sample() is a nullptr. Could you add it in here too just to be save?

charbeljc · 2021-07-21T19:55:56Z

I like this signal-based version for communicating changes in the envelope or sliders more.

However, some parts are now broken:
* Looping doesn't seem to work anymore and if the loop count is set to a value higher than 0 there are visual artifacts in the `DetailWaveDisplay`

What a mess... I guess I did let this PR in the middle of nowhere... Thank you for your review, I'm finally back on track.

* Pitch settings using Rubberband does not seem to take any effect either.

Are your working with the cli version or the library one ?

* Previously, when hitting play the white slider indicating current playback position was visible in the TargetWaveDisplay too

This should be restored now.

* (unrelated to this PR but I just noticed it) when increase the loop count to very large values, like 300, Hydrogen freezes. We probably should add some sanity checks in there

Hum, this don't hang on my machine, but yes, this must be addressed.

theGreatWhiteShark · 2021-09-26T18:53:22Z

Hey @charbeljc ,

Can you rebase this PR on master? It will be my next read.

charbeljc mentioned this pull request Jul 6, 2021

Further SampleEditor tweaks #1304

Open

7 tasks

charbeljc force-pushed the sample_editor_instant_feedback branch from 5b3532a to c8d619a Compare July 6, 2021 14:41

theGreatWhiteShark reviewed Jul 7, 2021

View reviewed changes

charbeljc force-pushed the sample_editor_instant_feedback branch from c8d619a to 6665f9e Compare July 7, 2021 20:51

charbeljc force-pushed the sample_editor_instant_feedback branch from 652edfb to 98ebd8f Compare July 15, 2021 21:31

sample editor: instant audiovisual feedback

0ea0925

Should close hydrogen-music#811 and hydrogen-music#1304

charbeljc force-pushed the sample_editor_instant_feedback branch from 98ebd8f to 0ea0925 Compare July 15, 2021 21:42

theGreatWhiteShark reviewed Jul 20, 2021

View reviewed changes

restore (almost) correct playback position tracking

8dc8ced

Merge branch 'development' into sample_editor_instant_feedback

a08527a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sample editor instant feedback #1314

Sample editor instant feedback #1314

charbeljc commented Jul 5, 2021

theGreatWhiteShark Jul 7, 2021

charbeljc Jul 7, 2021 •

edited

Loading

charbeljc Jul 8, 2021

theGreatWhiteShark Jul 8, 2021

charbeljc Jul 9, 2021

charbeljc Jul 9, 2021 •

edited

Loading

theGreatWhiteShark left a comment

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 19, 2021

theGreatWhiteShark Jul 20, 2021

theGreatWhiteShark Jul 20, 2021

theGreatWhiteShark Jul 20, 2021

charbeljc commented Jul 21, 2021

theGreatWhiteShark commented Sep 26, 2021

		@@ -51,18 +50,18 @@ static RubberBand::RubberBandStretcher::Options compute_rubberband_options( cons


		/* EnvelopePoint */
		EnvelopePoint::EnvelopePoint() : Object( EnvelopePoint::__class_name ), frame( 0 ), value( 0 )

		@@ -197,22 +253,24 @@ void SampleEditor::getAllFrameInfos()
		if ( pLayer ) {
		pSample = pLayer->get_sample();

		m_pTargetSampleView->move( 1, 1 );
		m_pTargetSampleView->updateDisplay( sample, 1.0 );

Sample editor instant feedback #1314

Are you sure you want to change the base?

Sample editor instant feedback #1314

Conversation

charbeljc commented Jul 5, 2021

Choose a reason for hiding this comment

charbeljc Jul 7, 2021 • edited Loading

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

charbeljc Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

theGreatWhiteShark 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

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

charbeljc commented Jul 21, 2021

theGreatWhiteShark commented Sep 26, 2021

charbeljc Jul 7, 2021 •

edited

Loading

charbeljc Jul 9, 2021 •

edited

Loading