Loading effects from amp or file does not preserve the position about plug

[offa]

While working on this, I've noticed many arrays of 4 effects which may need further investigation.

[offa]

Most parts of core slot changes are done; pre / post effects are handled differently now. You can get a work-in-progress version from Gitlab (245-fx_slots branch).

The progress is tracked in the related issue there (commenting here is still fine though 😉).

While the UI should handle slots correctly now, there are still some changes needed. Especially any file related operations need some more work and are likely broken at the moment – so please create backups before testing.

Good change to get rid of some technical dept btw..

[offa]

I have merged first iteration into master. Slots of 1-8 are used now.

[offa]

I have fixed broken shortcut slot connections on master can you reproduce the problem with that version?

[offa]

I see! Indeed, I can reproduce this.

From a first look: The new slot numbering has introduced conflicting shortcuts.

• Toggles: F1 to F8
• Defaulting: F5 to F12 (= slot + 5)

Now, usually there aren't enough F-Keys to map both, so we have to change the mapping.

How about leaving toggle on the F-keys (used more often?) and change 'load default' to Ctrl + F-Key?

[offa]

The shortcuts are as listed above now – the issue should be fixed now :-).

Can you have a look at the OS X button issue? Would be nice to get that one fixed too.

[j824h]

F1 to F8 work correctly.

Button shape issue doesn't seem fixed though, basically same as in #13 (comment).
I hope I can look into your attempted fix, but that might be possible some other days later.

[j824h]

I had not applied 628ffea and have been wondering why nothing changed. With the fix:

I guess this should look good enough.

[j824h]

Note on the vendor's design

An effect setting contains the position parameter ("POS" in the .fuse XML), ranging from 0 to 7.
The value encodes the order how the effect should be chained with other effect or the amplifier simulation.
The range is divided into 0-3, 4-7 so that amplifier simulation can be set between 2 groups of 4 effect slots.
The maximum number of effects used is 4 (when choosing one from all of the 4 types), so each group has sufficient slots to contain all used effects.

For the most natural representation, the interface should expose 8 slots.
This will generally give redundancy even when the maximum number of effects is used.

On the other hand, plug chose to use fx_pedal_settings::fx_slot ranging from 0 to 3, and fx_pedal_settings::position ranging from 0 to 1.

[offa]

I don't know what's the thought behind the current behaviour, but we should consider the vendors design instead.

So let's think about what needs to be changed:

• UI: 4 pre and 4 post amp slots (8 in total) – as Fuse does
• Slot value: Use 0-3 for pre and 4-7 for post amp
• DeSerialization: Use the new slot format; serialization may need some changes too
• Change load-from-file accordingly
• Remove Position enum since it's no longer needed
• ❓ is fx_pedal_settings.position still necessary?

[offa]

@j824h can you test whether order is preserved in your use cases now?

[j824h]

The order seems to be preserved, both when loading from a Mustang amp and loading from a file.

What breaks are the effect parameters, when loading from a file.
Every effect parameter seems to be set as 0 (default constructor?).

Other minor issues I find in cb2bf2c is that

1. F5, F6, F7, F8 shortcuts to toggle FX(5, 6, 7, 8) are not functioning, when the window for the corresponding FX(1, 2, 3, 4) is open,
2. (MacOS Qt specific) In the main window, the row of QPushButton: FxEffectButton(1, 2, 3, 4) looks inconsistent to EffectButton(1, 2, 3, 4) or Amplifier. The difference should come from whether minimumSize is set consistently.
   (https://doc.qt.io/qt-5/qpushbutton.html#:~:text=Note%3A%20On%20macOS)

Do you think those deserve their own issues, or just belong to this one as relating to 4 -> 8?

[offa]

Thanks for your feedback.

What breaks are the effect parameters, when loading from a file.
Every effect parameter seems to be set as 0 (default constructor?).

The loading logic is really wired at some places, I'll have a look at it. (Stupid question, I know) This was working with the old IDs?

(MacOS Qt specific) In the main window, the row of QPushButton: FxEffectButton(1, 2, 3, 4) looks inconsistent to EffectButton(1, 2, 3, 4) or Amplifier. The difference should come from whether minimumSize is set consistently.
(https://doc.qt.io/qt-5/qpushbutton.html#:~:text=Note%3A%20On%20macOS)

I do not have a system to reproduce this, so I might need your testing

Do you think those deserve their own issues, or just belong to this one as relating to 4 -> 8?

I'm fine with both. We can create separate issues at any time :-)

[offa]

Regarding Mac OS buttons: Could you have a look at 266-osx_buttons branch? I have changed the FX loop buttons to same minimum size as the others. And increased minimum height to 30 as mentioned in the docs.

[j824h]

Regarding Mac OS buttons: Could you have a look at 266-osx_buttons branch? I have changed the FX loop buttons to same minimum size as the others. And increased minimum height to 30 as mentioned in the docs.

Now I like that every button for effects is consistently round-shaped in the minimal window size.

When the window height is increased though, vsizetype="Expanding" will bring the old situation back.

Even if the stable consistency is what we can agree on, it seems the fix will depend on aesthetic decisions.
So let's put this out of this issue and for later debate...

[offa]

Even if the stable consistency is what we can agree on, it seems the fix will depend on aesthetic decisions.
So let's put this out of this issue and for later debate...

I'd expect it should stay consistent over all sizes. Or is this style change common on Mac applications (I wouldn't expect)?

What I tried to get are all buttons square … seems we need another iteration here.

What's strange to me: The corners become square with increased size, but accoring to docs it should be oppsoiste:

Note: On macOS when a push button's width becomes smaller than 50 or its height becomes smaller than 30, the button's corners are changed from round to square. Use the setMinimumSize() function to prevent this behavior.

[offa]

Next iteration: I have updated the branch to use same size policy for FX buttons.

[j824h]

The issue seems persisting. For example, F5 works when FX1 window is off, but doesn't seem to do anything when FX1 window is on.

[offa]

I have tried to reproduce it with these steps:

1. Open Plug with Effect 1 and Effect 5 empty
2. Press Ctrl+1 --> Effect 1 window opens and active
3. Press Ctrl+5 --> Effect 5 window opens and active
4. Close 5 Window and Press Ctrl+5 --> Effect 5 window opens again
5. Close both
6. Press Ctrl+5 --> Effect 5 window opens
7. Press Ctrl+1 and set "off"
8. Press Ctrl+5 --> Effect 5 window opens
9. Press Ctrl+1 and set "on"
10. Press Ctrl+5 --> Effect 5 window opens
11. Pressing either Ctrl+1 or Ctrl+5 show their corresponding windows

Everything works as expected though. Where does it stop working when you the steps?

[j824h]

You mean Ctrl + n? I meant Fn.

[offa]

The shortcuts are mapped as Ctrl key and a number. According to qt docs things are a little different on OS X, which uses different modifiers.

[j824h]

I have tried to reproduce it with these steps:

1. Open Plug with Effect 1 and Effect 5 empty
2. Press Ctrl+1 --> Effect 1 window opens and active
3. Press Ctrl+5 --> Effect 5 window opens and active
4. Close 5 Window and Press Ctrl+5 --> Effect 5 window opens again
5. Close both
6. Press Ctrl+5 --> Effect 5 window opens
7. Press Ctrl+1 and set "off"
8. Press Ctrl+5 --> Effect 5 window opens
9. Press Ctrl+1 and set "on"
10. Press Ctrl+5 --> Effect 5 window opens
11. Pressing either Ctrl+1 or Ctrl+5 show their corresponding windows

Everything works as expected though. Where does it stop working when you the steps?

I'm sorry that I might have confused you, but I really meant Fn.
They're supposed to do 'set "off"' and 'set "on"' in your step 7 & 9.

plug/src/ui/effect.cpp

Line 135 in a38ded4

QShortcut* off = new QShortcut(QKeySequence(QString("F%1").arg(slotArg)), this, nullptr, nullptr, Qt::ApplicationShortcut);

I somehow have discovered the shortcuts, they worked for F1–F4, and now I see where they come from + why they don't work any more.
I thought we should be supporting this.
What do you think?

[offa]

@j824h what's still open here?