Instrument editor love #1301

charbeljc · 2021-06-14T22:39:50Z

Ok, second batch for the instrument editor, which become finally mostly usable 😁

On the technical side, wave widgets now track mouse movements continuously, this allow us to highlight envelope points before we drag / delete them, and see why editing start/loop/end frames in main widget is broken (final PR to come 😀 )

@theGreatWhiteShark if you could have a look ...
Regards.

charbeljc · 2021-06-15T09:16:00Z

I have one more commit pending before this pull request is complete ...

* deleting first and last point is only allowed when they are the last ones. * deleting the first or last point deletes the other end. * clicking on a wave with an empty envelope inserts to points at both ends

* align logic between SampleEditor & MainSampleWaveDisplay * don't change slider while dragging so that when you start editing start frame, you don't mess with end frame jumps.

charbeljc · 2021-06-15T10:20:16Z

Ok, I'm done with Sample Editing 😀

theGreatWhiteShark · 2021-06-15T14:39:24Z

@theGreatWhiteShark if you could have a look ...

Ok, I'm done with Sample Editing grinning

That's quite fast 😄 . I'll try to take a look within the next days.

theGreatWhiteShark · 2021-06-17T07:11:44Z

Hey @charbeljc ,

As these are improvements rather than bug fixes could you change the target to branch development? (Sorry for the inconvenience. This will be straight more straight forward once 1.1 is release and we use master for incremental improvements again)

You do not need to close this one and open a new one but can just press the "Edit" button to the right of the title of this PR and select a different branch via a combo box.

theGreatWhiteShark

Thanks for your contribution! I like it a lot!

Could you also have a look at the coding conventions in the DEVELOPERS file in our repo? I know that our code right now is quite a mess. But if we all stick to the same conventions it will become more easily readable in the future.

I also found two further inconsistencies in the Sample Editor (which are probably not related to your PR)

open the SampleEditor -> enter a large value in the spinbox of the "Start" and press enter -> both the values in the spinboxes of "Start" and "Loop" get updated but only the "S" line gets moved in the wave display while "L" remains at zero.
right to the "Play original sample" button there is a the label "new sample length" which seem to show incorrect values until the "E" marker is moved.

theGreatWhiteShark · 2021-06-17T11:16:19Z

src/gui/src/SampleEditor/SampleEditor.h

+ void setUnclean();
+ void setClean();
+
+ bool m_bSampleEditorClean;


How about making this variable private? Also, could you initialize it in the constructor of SampleEditor?

done, see 397034a

theGreatWhiteShark · 2021-06-17T11:21:33Z

src/gui/src/SampleEditor/SampleEditor.cpp

 }

 void SampleEditor::valueChangedStartFrameSpinBox( int )
 {
 testpTimer();
 m_pDetailFrame = StartFrameSpinBox->value();
+ if (m_pDetailFrame == __loops.start_frame) { // no actual change
+ if (! m_bAdjusting ) on_PlayPushButton_clicked();


Nice feature. I like it!

theGreatWhiteShark · 2021-06-17T11:30:29Z

src/gui/src/SampleEditor/SampleEditor.cpp

 QApplication::restoreOverrideCursor();
+ InstrumentEditorPanel *panel = InstrumentEditorPanel::get_instance();
+ panel->selectLayer( panel->getSelectedLayer() ); // reselect layer to trigger update


AFAIU the purpose of this line is to call the updateDisplay( panel->getSelectedLayer() ) method of the m_pWaveDisplay member in the InstrumentEditor, right? How about introducing a new public member, like InstrumentEditorPanel::updateWaveDisplay( int ), that does the job more explicitly?

see 499fbca

theGreatWhiteShark · 2021-06-17T11:33:37Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.h

 bool m_bStartSliderIsMoved;
 bool m_bLoopSliderIsMoved;
 bool m_bEndSliderIsmoved;

+ Slider m_SelectedSlider;


Could you initialize the variable in the constructor?

done, see 7106762

theGreatWhiteShark · 2021-06-17T11:41:02Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.cpp

 update();
+ if (propagate()){


Doesn't in the current implementation returnAllMainWaveDisplayValues() and thus also propagate() do always return true?

Hum, you are right ...

see cb9866a

theGreatWhiteShark · 2021-06-17T12:00:15Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.cpp

 }

 //loopframeposition
- else if (ev->y()<=65 ) {


Just to be sure. Due to this changes dragging the start marker further than the loop and end marker will move the latter ones while the end marker can only move the loop one and the loop one can move none. Is this intentional?

Yes, this was for consistency with the spinbox behavior. When you try to decrease loop_frame before start_frame, the spinbox clip it at start_frame. Let me know what you think about it.

Ah, I see. I think it's fine.

theGreatWhiteShark · 2021-06-17T12:19:04Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.cpp

@@ -283,3 +303,26 @@ void MainSampleWaveDisplay::mouseReleaseEvent(QMouseEvent *ev)



+
+void MainSampleWaveDisplay::chooseSlider(QMouseEvent * ev)


How do you feel about the y-coordinate based selection approach for the sliders that was implemented previously? I like your approach using the Manhatten distance a lot more but am still a little bit confused about it's y component. But I never really used the SampleEditor in a productive way so I do not know whether the vertical selection emulating the grabbing of the labels is necessary/handy.

For the y target coordinates, I choose mostly the y location of the slider's labels (S, L, E), also, the slider selection is done on mouse press and never change while dragging.

Before, the selected slider could change while dragging, furthermore, there was a dead zone between each slider influence.

The previous behavior could you drive nuts ...

Definitely. It's a huge improvement. The only thing I found a little bit strange was that clicking in the space between two adjacent sliders in order to move one of them there yields different results depending on the y coordinate of the mouse pointer.

Definitely. It's a huge improvement. The only thing I found a little bit strange was that clicking in the space between two adjacent sliders in order to move one of them there yields different results depending on the y coordinate of the mouse pointer.

Yeah, but I think we can get used of this behavior. At least we can make a mental model from it, and having the actual slider selected highlighted helps a lot.

theGreatWhiteShark · 2021-06-17T12:41:24Z

src/gui/src/SampleEditor/TargetWaveDisplay.cpp

+ painter.setPen( QPen(lineColor, 1 , Qt::SolidLine) );
+ painter.drawLine( envelope[i]->frame, envelope[i]->value, envelope[i + 1]->frame, envelope[i +1]->value );
+ if ( i == selected )
+ painter.setBrush( selectedColor );


How about to draw the value of the selected point in here as well? This way it just needs to be hovered and not clicked for the user to inspect its value.

done, see 2b6ff4c

theGreatWhiteShark · 2021-06-17T12:52:10Z

src/gui/src/SampleEditor/TargetWaveDisplay.cpp

+ // if only 2 points, remove them both
+ envelope.clear();
+ } else {
+ envelope.erase( envelope.begin() + m_nSelectedEnvelopePoint );


This statement needs to be wrapped in an if ( m_sSelectedEnvelopePoint != -1 ) or Hydrogen will segfault when right clicking in an empty part of the TargetWaveDisplay.

Good catch 😁, see 29edd84

theGreatWhiteShark · 2021-06-17T12:53:27Z

src/gui/src/SampleEditor/TargetWaveDisplay.cpp

+ update_envelope(envelope, m_nSelectedEnvelopePoint, m_nX, m_nY, m_nSnapRadius);
+ }
+ } else if (ev->button() == Qt::RightButton ) {
+ //remove point


How about setting m_sInfo = "" when right clicking to etablish a feeling of "nothing happens" when right clicking in an empty part of the TargetWaveDisplay?

done also, see 2b6ff4c

charbeljc · 2021-06-17T16:58:46Z

@theGreatWhiteShark Thanks for your review, I addressed most of your comments and I should be really done this time.
Let me know what you think and I will rebase this PR on development branch. Regards,

charbeljc · 2021-06-17T17:06:23Z

Thanks for your contribution! I like it a lot!

Could you also have a look at the coding conventions in the DEVELOPERS file in our repo? I know that our code right now is quite a mess. But if we all stick to the same conventions it will become more easily readable in the future.

I also found two further inconsistencies in the Sample Editor (which are probably not related to your PR)
* open the SampleEditor -> enter a large value in the spinbox of the "Start" and press enter -> both the values in the spinboxes of "Start" and "Loop" get updated but only the "S" line gets moved in the wave display while "L" remains at zero.

* right to the "Play original sample" button there is a the label "new sample length" which seem to show incorrect values until the "E" marker is moved.

I'll have a look at this later ! (and also, I will try to follow developper's style guide)

theGreatWhiteShark

Nice.

There are still some oddities but they were most probably already present so you do not need to fix them.

the aforementioned: open the SampleEditor -> enter a large value in the spinbox of the "Start" and press enter -> both the values in the spinboxes of "Start" and "Loop" get updated but only the "S" line gets moved in the wave display while "L" remains at zero.
Open the SampleEditor -> change something and hit the "Close" button -> a dialog "Unsave changes..." will pop up and the user has to consent to discard the changes -> clicking "Ok" and reopen the SampleEditor by clicking "Edit Layer" again -> the same popup shows up again. I think the second one is a dud.
When entering a fresh instance of the SampleEditor the value of the EndFrameSpinBox looks perfectly fine. But as soon as the mouse pointer enters the MainSampleWaveDisplay it is reset to 0 by SampleEditor::getAllFrameInfos() what causes some other oddities. The slider, however, stays at it's correct position.
The first value change in the Start, Loop, and End spinboxes is discarded and, instead, the slider is shown in the DetailWaveDisplay. It would be nice to update the DetailWaveDisplay as soon as the spinboxes have focus and make the first change affect the value as well.

But again: this is more a list of things I noticed that still need to be fixed and not a list of things you have to do for this PR to get merged. 😉

theGreatWhiteShark · 2021-06-20T13:13:56Z

src/gui/src/SampleEditor/MainSampleWaveDisplay.cpp

 update();
+ mouseUpdateDone();


How come you mark the state of the SampleEditor unclean once the user enters the MainSampleWaveDisplay using the mouse? This makes the "Apply Changes" not flat and indicates something has changed. Changes via drag should be already covered by the call of mouseUpdateDone in mousePressEvent

How come you mark the state of the SampleEditor unclean once the user enters the MainSampleWaveDisplay using the mouse? This makes the "Apply Changes" not flat and indicates something has changed. Changes via drag should be already covered by the call of mouseUpdateDone in mousePressEvent

Hum, actually, it does not mark the editor as changed, only setClean()/setUnclean() does that. I should maybe rename mouseUpdateDone() to something like mouseActionDone()

theGreatWhiteShark · 2021-06-20T13:39:19Z

In addition, could you add all the changes in the UX to the ChangeLog once you rebased on development?

charbeljc · 2021-06-23T14:19:13Z

In addition, could you add all the changes in the UX to the ChangeLog once you rebased on development?

Ahem, the ChangeLog... Those are my weak point... 😄

I just rebased this PR on development branch, see #1302. I would be glad if it could be merged soon, because I have interesting things to come, namely python bindings for the core library, UI as a shared library, with python bindings also, and it starts to make quite a big stack of commits 😁

See #1303 for the bindings 😀

theGreatWhiteShark · 2021-06-24T17:58:19Z

I just rebased this PR on development branch, see #1302. I would be glad if it could be merged soon, because I have interesting things to come,

Sure.

See #1303 for the bindings grinning

Nice!

charbeljc force-pushed the instrument_editor_love branch from 361f968 to 9ed1c7d Compare June 15, 2021 09:10

charbeljc marked this pull request as draft June 15, 2021 09:15

charbeljc marked this pull request as ready for review June 15, 2021 10:18

charbeljc added 15 commits June 15, 2021 12:19

wave editors: always track mouse movements

d4745a1

remove code duplication for envelope painting

ad21827

don't move !

a7bace4

using references instead of pointers is nicer

efded62

add EnvelopeEditMode enum, more refs instead of pointers

ac281b9

highlight selected/selectable envelope point on mouse moves

eebfe81

highlight selected frame slider in MainSampleWaveDisplay

20289a8

use manhattanLength() to choose frame slider

914c56b

envelope editing: dont stop dragging when mouse leave widgets

c10aaa2

allow empty envelopes

e5c6490

* deleting first and last point is only allowed when they are the last ones. * deleting the first or last point deletes the other end. * clicking on a wave with an empty envelope inserts to points at both ends

make the main wave widget more intuitive

5e689e1

* align logic between SampleEditor & MainSampleWaveDisplay * don't change slider while dragging so that when you start editing start frame, you don't mess with end frame jumps.

show envelope value only when dragging

bff630e

update layer's wave preview on change

6ada7ed

hit enter everywhere in editor to play sample

a70c1d8

use manhattan distance for envelope point selection

4e5fc43

charbeljc force-pushed the instrument_editor_love branch from 8b4c80a to 4e5fc43 Compare June 15, 2021 10:19

theGreatWhiteShark reviewed Jun 17, 2021

View reviewed changes

charbeljc added 6 commits June 17, 2021 16:28

avoid crash when no point is selected for removal

29edd84

make SampleEditor::m_bSampleEditorClean private & initialize it in ctor

397034a

add public method InstrumentEditorPanel::updateWaveDisplay()

499fbca

initialize MainSampleWaveDisplay::m_SelectedSlider in ctor

7106762

show envelope point value on hover

2b6ff4c

highlight first and last point also when selected

68f17d1

charbeljc added 2 commits June 17, 2021 18:52

factorize some code in MainSampleWaveDisplay

cb9866a

merge envelope points at same frame

e259240

naming conventions

03b9c18

charbeljc force-pushed the instrument_editor_love branch from a257d8e to 03b9c18 Compare June 17, 2021 17:41

theGreatWhiteShark reviewed Jun 20, 2021

View reviewed changes

charbeljc mentioned this pull request Jun 23, 2021

Sample editor love (on dev branch) #1302

Merged

charbeljc closed this Jun 23, 2021

charbeljc deleted the instrument_editor_love branch June 24, 2021 20:44

		@@ -283,3 +303,26 @@ void MainSampleWaveDisplay::mouseReleaseEvent(QMouseEvent *ev)




		void MainSampleWaveDisplay::chooseSlider(QMouseEvent * ev)

Instrument editor love #1301

Instrument editor love #1301

Conversation

charbeljc commented Jun 14, 2021

charbeljc commented Jun 15, 2021

charbeljc commented Jun 15, 2021

theGreatWhiteShark commented Jun 15, 2021

theGreatWhiteShark commented Jun 17, 2021

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

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 Jun 17, 2021

charbeljc commented Jun 17, 2021

theGreatWhiteShark left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theGreatWhiteShark commented Jun 20, 2021

charbeljc commented Jun 23, 2021 • edited Loading

theGreatWhiteShark commented Jun 24, 2021

theGreatWhiteShark left a comment •

edited

Loading

charbeljc commented Jun 23, 2021 •

edited

Loading