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

Multi-Unit View #181

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Multi-Unit View #181

wants to merge 2 commits into from

Conversation

qm210
Copy link
Contributor

@qm210 qm210 commented Nov 10, 2024

first implementation of #173

didn't test it that thoroughly yet.

there is at least one problem I couldn't solve:
the column widths get updated on the fly (because I need to have rendered every parameter widget at least once in order to know how wide they are), and thus, when the Button (bottom right) is first clicked after startup, the view can not scroll to the correct x position.

The width calculations also lead to some short "flickering" when each unit is rendered the first time, I didn't see an easy way around that

image

also, no idea how to make that tooltip stay inside the window.

PS: I also sneaked in the changelog edits from my last pull request, that I forgot - if you prefer these in an extra PR, I can do so ;)

@vsariola
Copy link
Owner

I have been travelling and didn't have time to look into this. I hope to have time next weekend.

@vsariola
Copy link
Owner

I added the lines to CHANGELOG.md for the past commits that were forgotten. But the rest of this change is a bit intimidating with so many changes :D I'll try to read carefully what is going on.

Copy link
Owner

@vsariola vsariola left a comment

Choose a reason for hiding this comment

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

While I like the idea, the UI can be pretty confusing now. For example:

image

The texts indicate that "Outaux" is selected but then there is Envelope unit there shown. Also the fact that the units are vertical in the instrument list and the horizontal in the multiunit view bothers me quite a bit, but I can see why you did it like that.

Do we really need to have both the multiunit view and the single unit view?
Rather than having several different GUI modes, I'd prefer the multiunit mode to be so intuitive that it can be the only mode and we can save some code and some user confusion by not having multiple different modes.

Did you consider the split view similar to VSCode? There's this button that splits the view in the top right corner of the VSCode:

image

Like so:
image

The disadvantage is that there would be more space wasted on the unit list if it's repeated every view. But the advantage is that it would be far less confusing as you see exactly which unit you are on the unit list, like it is now. The implementation would be something like that we have struct e.g. like InstrumentView

type InstrumentView {
  InstrIndex, InstrIndex2 int
  UnitIndex, UnitIndex2   int
  ParamIndex              int
  UnitSearchIndex         int
  UnitSearchString        string
  UnitSearching           bool
}

and the Model would have slice with 1 or more InstrumentViews. Then on the gioui side generate a new InstrumentEditor for each InstrumentView. Just a thought, not sure how massive rewrite this would be.

Then a couple of code related things:

  1. Callbacks are not nice. Instead, the gioui / imgui way is that the widgets should indicate that something has happened (like Clickable.Clicked()), which is called by whoever is calling the Layout, and there we can take appropriate action in response to things having changed.
    Related: we really should refactor so that Layout and Update methods are separate from each other for each widget / editor. Update should do all input handling and deciding how to respond to those, and Layout would do only the layout. Update can also return (event, ok) where events are things that might be interesting to the higher level. For example, draglist could somehow signal that selected item was changed etc. I think this refactoring can be later, but the Layout methods are becoming beasts.

  2. Left and right arrow keys don't seem to change the parameters anymore.

  3. The colors indicating what is selected is a bit weird, e.g.:

image

What is selected here?

@@ -168,6 +168,9 @@ type Clickable struct {
history []widget.Press

requestClicks int

// optional callback for custom interactions
OnClick func()
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not comfortable with adding callback for Clickable; the imgui way of doing this should be for whoever called Layout to check for Clicks with something like:

for widget.Clicked() {
   doSomething();
}
...
widget.Layout(...)
```

I am thinking of refactoring also away the fixed binding of buttons to actions, but I have not found the way to do that yet.

@@ -31,6 +31,7 @@ type DragList struct {
swapped bool
focused bool
requestFocus bool
onSelect func(index int)
Copy link
Owner

Choose a reason for hiding this comment

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

similarly, I don't like callbacks for widgets when something happens. Instead, we could have something like:

for event,ok := dragList.Update(gtx); ok; event,ok = dragList.Update(gtx.) {
   // draglist.Update can return different kinds of events to tell what has happened, like Clickable.Update returns clicks
}
...
dragList.Layout(...)

@@ -105,6 +105,11 @@ func NewInstrumentEditor(model *tracker.Model) *InstrumentEditor {
ret.linkDisabledHint = makeHint("Instrument-Track\nlinking disabled", "\n(%s)", "LinkInstrTrackToggle")
ret.linkEnabledHint = makeHint("Instrument-Track\nlinking enabled", "\n(%s)", "LinkInstrTrackToggle")
ret.splitInstrumentHint = makeHint("Split instrument", " (%s)", "SplitInstrument")
ret.unitDragList.onSelect = func(index int) {
Copy link
Owner

Choose a reason for hiding this comment

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

This goes away when getting rid of the callback and making it index,ok := dragList.Selected()

@@ -386,7 +392,7 @@ func (ie *InstrumentEditor) layoutUnitList(gtx C, t *Tracker) D {
if ie.searchEditor.requestFocus {
// for now, only the searchEditor has its requestFocus flag
ie.searchEditor.requestFocus = false
gtx.Execute(key.FocusCmd{Tag: &ie.searchEditor.Editor})
gtx.Execute(key.FocusCmd{Tag: ie.searchEditor.Editor})
Copy link
Owner

Choose a reason for hiding this comment

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

This does not look correct to me. It's the address of the widget that is always its tag by convention. ie is a pointer, searchEditor is a pointer, but Editor is a field, so we should take its address. Are you sure about this?

}
ret.caser = cases.Title(language.English)
ret.copyHint = makeHint("Copy unit", " (%s)", "Copy")
ret.disableUnitHint = makeHint("Disable unit", " (%s)", "UnitDisabledToggle")
ret.enableUnitHint = makeHint("Enable unit", " (%s)", "UnitDisabledToggle")
ret.multiUnitsHint = "Toggle Multi-Unit View"

ret.MultiUnitsBtn.Clickable.OnClick = func() {
Copy link
Owner

Choose a reason for hiding this comment

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

No callbacks here either. Instead when updating the widget, check Clickable.Clicked() and take some action if it happened.

@vsariola
Copy link
Owner

Alternative idea: we put all the units in a vertical list, but we flow sliders so that if two sliders fit on the same line we put them horizontally. This way all the units are always vertically, both in the left list and in the right list.

The list could go on even past an instrument, so you can scroll up and down to see all the parameters of a unit.

Oh, and we would save a lot of space by using knobs instead of sliders, maybe that way it would not be so awkward to have all the knobs of a unit horizontally?

@LeStahL
Copy link
Contributor

LeStahL commented Nov 16, 2024

I love the idea with using knobs instead of sliders!

@vsariola
Copy link
Owner

vsariola commented Nov 16, 2024

I love the idea with using knobs instead of sliders!

I think there's a certain "rack modules" vibe with having knobs horizontally and units stacked vertically

@LeStahL
Copy link
Contributor

LeStahL commented Nov 16, 2024

I always found this kind of presentation very appealing - https://www.beat.de/media/beat/styles/tec_frontend_large/public/images/2020/06/10/image-76056--30523.png
(obviously without the "tight packing to fit the least amount of eurorack units" here) :)

@vsariola
Copy link
Owner

Maybe we could even take the rack metaphor further and draw lines to indicate how units connect signals to each other.

@vsariola
Copy link
Owner

Finally, for people with ultrawide monitors, ability to split view would stillbe useful. But the split view would essentially duplicate the entire instrument editor.

@LeStahL
Copy link
Contributor

LeStahL commented Nov 16, 2024

Maybe make a "horizontal rack" switch for the ultrawidescreen ppl? :)

@vsariola
Copy link
Owner

I think it'll be almost trivial to draw line rails on the left hand side, with the line shifted to the right equal to its position in the stack so they don't overlap. Of course, works too if all units are horizontal. But having them flowing so that sometimes they are horizontal and sometimes vertical is a problem for drawing those lines.

@vsariola
Copy link
Owner

Let's move the discussion back to #173 because I think we're diverging from what this pull request is.

@qm210
Copy link
Contributor Author

qm210 commented Nov 22, 2024

(originally posted in #173 but it better belongs here)

Hey, I like these ideas in general (or indeed, everything that goes towards "more visibility of what is going on" - especially on smaller screens), but am not sure, what, from this discussions, are the real blockers for the pull request and what are new issues for future refinement :)

could we clarify?

@vsariola
Copy link
Owner

vsariola commented Dec 3, 2024

Hey, I like these ideas in general (or indeed, everything that goes towards "more visibility of what is going on" - especially on smaller screens), but am not sure, what, from this discussions, are the real blockers for the pull request and what are new issues for future refinement :)

could we clarify?

So, to clarify: I would prefer not merging this. To me, this is taking the GUI to a wrong direction, as it add complexity to code and confusion to user, without entirely solving the needs. Specifically:

  1. Complexity: two different edit modes, which adds more mental load. A single good edit mode would be preferred.
  2. Confusion: the units are top-to-bottom on the list on summary list, but left-to-right on the large list
  3. Does not solve the problem: the two biggest needs for having multiple units editable at the same time are a) Editing sender and send target unit at the same time. These could be even in different instruments. b) Editing "Global" and a "Instrument" at the same time.

The approach in this PR does not solve b, and only solves a in the limited case that the units are in the same instrument and quite close to each other in the unit list. But quite often, even within an instrument, you have all the synthesis done first, finishing with outaux, and then there's envelope->send(pop) combos in the end to make some modulations. These sends are already several units away from the modulated unit, so probably don't fit on the same view.

Maybe we could merge this PR as a "temporary fix" and later change everything according to the whatever plan gets agreed on in #173, but I'd prefer we skip temporary solutions and try to go directly towards whatever we think will be ultimately the best.

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.

3 participants