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

add 8 sampler and small sampler option to latenight #1840

Merged
merged 5 commits into from
Nov 3, 2018

Conversation

nopeppermint
Copy link
Contributor

This add's the option to have 4 or 8 sampler's in skin latenight

4samplers
8samplers

@ronso0
Copy link
Member

ronso0 commented Oct 13, 2018

While I've been fancying this for a while, too, I believe we also need minimal samplers like in the other skins.
Maybe we can put a expand/collapse button in between the left-hand & right-hand samplers and pick up the style of the respective button in the FX units?

@ronso0
Copy link
Member

ronso0 commented Oct 13, 2018

"Load/Save sampler bank" buttons can go to SAMPLERS section.
MISCELLANEOUS can become LIBRARY now.

@nopeppermint
Copy link
Contributor Author

we also need minimal samplers like in the other skins.

that's my next step ;-)

"Load/Save sampler bank" buttons can go to SAMPLERS section.
MISCELLANEOUS can become LIBRARY now.

I agree

@nopeppermint
Copy link
Contributor Author

smal_and_big_sampler

@nopeppermint
Copy link
Contributor Author

finished

Finished, please Test!

@nopeppermint nopeppermint changed the title add 4 sampler to latenight add 8 sampler and small sampler option to latenight Oct 13, 2018
@Be-ing
Copy link
Contributor

Be-ing commented Oct 15, 2018

Most all-in-one style controllers have one or two rows of 4 sampler buttons on each side, which is why we have rows of 8 in the other skins. I'd prefer to update LateNight to use that layout.

@ronso0
Copy link
Member

ronso0 commented Oct 15, 2018

Most all-in-one style controllers have one or two rows of 4 sampler buttons on each side, which is why we have rows of 8 in the other skins.

This is only true for Tango & Deere, and 8 samplers only fit into Deere (at minimal size) because the hotcue buttons are missing. Squeezing 8 samplers per row into LateNight's current layout, and with its big buttons everywhere, would require a massive rework of the expanded sampler template.
I consider this PR a good first step to get more samplers at all.

Later on, we can think about working around the space limitations while considering the 'common' controller layout. For example we could have 8 minmal samplers per row, separated into 1-4 and 5-8, then expand each block of 4 samplers by switching back to a 2x2 grid.

@ronso0
Copy link
Member

ronso0 commented Oct 15, 2018

The expand button seems kinda lost in between the samplers blocks. I suppose it could expand to the sampler row's height. This way it would stay in the same place under the mouse cursor when expanding/collapsing a row.
Also, I feel the button's drop shadow only makes sense in brighter button containers (like decks etc). I suggest to omit it (the bg-image) here and use a gradient for the button border instead. I'd also try to replace the +/- icons with text labels like 1-4 and 5-8. That would make the buttons wider but it would also pick up the descriptive FX expand buttons' style.

@nopeppermint
Copy link
Contributor Author

now there are more options:

if you have only 4 samplers (no changes here):
=> 4 small Samplers in a row
=>4 big Samplers in a row

if you have 8 samplers:
=> 8 tiny Samplers in a row

8_sampler_per_row

=> either the Samplers 1-4 big in a row
=> or the Samplers 5-6 big in a row

TODO:
=> change the "+/-" Button to "+/- 1-4" and "+/- 5-8"
=> maybe change color of Sampler 5-8 to the same as deck 3/4
(how can this be done easily?
adding color: #09B2AE; somewhere..)

@ronso0
Copy link
Member

ronso0 commented Oct 16, 2018

I'm sorry to say the last commit feels like a regression. The toggling is intransparent, samplers are suddenly gone when expanding others, mini samplers' Play button is too small (IMO should be sized like the preview deck's Play button)

What about making 8 samplers the standart? The foundation is already there:

  • let's drop the complicating '4/8 samplers' option
  • mini samplers a re fine already (except (1)
  • let's expand sampler rows like this (IMO all 8 samplers should be expanded, small samplers 5-8 are just for illustration):
    latenight-8samplers-alt

(1) I think we can split the space in the small samplers (next to Play) into two rows: top row shows just the title, while the bottom rows host BPM, Eject, xFade orientation (and maybe other controls)

What do you think?

@nopeppermint
Copy link
Contributor Author

let's drop the complicating '4/8 samplers' option

ok

I think we can split the space in the small samplers (next to Play) into two rows: top row shows just the title, while the bottom rows host BPM, Eject, xFade orientation (and maybe other controls)

done

let's expand sampler rows like this (IMO all 8 samplers should be expanded, small samplers 5-8 are just for illustration):

I do not like the samplers expanded over 2 rows, I prefer the current solution, either the left 4 or the right for samplers can be expanded, not both.

Maybe you can add this 2x2 grid layout as an alternative expanding solution later on (as I have no idea how to get this layout done).

The advantage on my expanding solution is that we somehow keep the skins similar. There might be a lot of users outside which like the current layout of their 4 samplers. Now they have the option to collapse them and use 8 samplers parallel.

=> I like to change the color of Sampler 5-8 to the same as deck 3/4

How can this be done easily?
adding color: #09B2AE; somewhere..

@nopeppermint
Copy link
Contributor Author

8smallsampler
1-4expanded
5-8expanded

@nopeppermint
Copy link
Contributor Author

@ronso0
any Idea how to get the "bpm" thing aligned left?

@ronso0
Copy link
Member

ronso0 commented Oct 17, 2018

Maybe it was to early to drop the 4/8 samplers option, because now right now the UX feels wrong:

  • we have regular samplers 1-4 visible by default (empty config, clean install)
  • only after collapsing samplers 1-4 it's revealed samplers 5-8 are also available
  • the other 4 samplers (either 1-4 or 5-8) are inaccessible from the skin which is problematic/confusing especially if a) they are playing because they were activated just before pressing an expand button b) they are mapped to a controller and were loaded by the user, but now their on-screen counterpart is completely missing
  • only after switching through samplers 1-4, samplers 5-8 and finally mini samplers 1-8 users can understand how it works in LateNight
  • the middle spacer needs to be wider

I'm a bit torn apart how to proceed from here. I respect the case that there users familiar with the layout of 4 full-featured samplers who like to keep it that way. OTOH we need to consider new users checking out all skins to whom it should be obvious that we now have 8 samplers in LateNight as well.

This is my sugestion to keep the samplers layout consistent with the other skins as well as nicely accessible from skin and controllers:

  • 8 samplers are the default
  • small samplers are the default
  • skin menu allows to switch back to 4 samplers
  • all samplers are expanded at once
  • 4 samplers expand to 1x4 (like before this PR)
  • 8 samplers expand to 2x2 (1-4) and 2x2 (5-8) side by side (matches Shade skin where we also have 8 samplers in two 2x2 grids), emphasize the separation 1-4 / 5-8 with a wider spacer

@ronso0
Copy link
Member

ronso0 commented Oct 17, 2018

any Idea how to get the "bpm" thing aligned left?

Right now, the decks' Orientation, Keylock and Eject icons are used for the samplers. Those icons are defined inline (PushButton) and therefore are 18x25px each so they are equally distributed below the star rating.
In the samplers we need them to be compact. Therfore I suggest to duplicate those icons, resize them to 17x18 or 18x18 and save them as sampler-specific versions (i.e. btn_keylock_sampler...) Then you'd just need to pack an expanding spacer (min,min) in between the BPM and the fixed-size sampler control icons. BPM is already aligned left (note how the sampler expands if BPM has three digits); except the sampler is empty, then WNumber expands to available space and the "0.00" string moves to the right.

Btw: the orientation icon is brighter than keylock & eject.

@nopeppermint
Copy link
Contributor Author

matches Shade skin where we also have 8 samplers in two 2x2 grids
@ronso0
in shade it's like that (correct me if I am wrong):
Samplers:
1234
5678

That's basically what we had in latenight but with two row's.

But what you and @Be-ing want is:
Samplers:
12 56
34 78

right?

This is expansion on the left (1-4) and expansion on the right (5-8) similar to latenight effect rack's

@nopeppermint
Copy link
Contributor Author

- all samplers are expanded at once

I overlooked this one, If there is only one expansion the realisation is much easier.
So the only open point for me is then the position of the samplers 1-8 as mention above.

@nopeppermint
Copy link
Contributor Author

duplicate those icons, resize them

I agree

@ronso0
Copy link
Member

ronso0 commented Oct 17, 2018

In shade it's like that (correct me if I am wrong)

I do ;)
In Shade it's actually
12 56
34 78
That's what matches most controllers where you'd have at least one row of 4 sample triggers per side

@Be-ing
Copy link
Contributor

Be-ing commented Oct 19, 2018

Samplers:
12 56
34 78

No,
1-2-3-4; 5-6-7-8
9-10-11-12; 13-14-15-16

@ronso0
Copy link
Member

ronso0 commented Oct 19, 2018

Samplers:
12 56
34 78

No,
1-2-3-4; 5-6-7-8
9-10-11-12; 13-14-15-16

Right now, we're talking about 8 samplers. So it's the same:
left side 1 2 3 4 // right side: 5 6 7 8
in expanded view, they're stacked exactly as mentioned (and as samplers are laid out in Shade)

1 2 | 5 6
3 4 | 7 8
and that works well with controllers with 4 sampler buttons in a row.

@nopeppermint
Copy link
Contributor Author

@ronso0 @Be-ing
The basic concept of 8 small samplers, with option to show only 4 samplers as backwards compatibility (4 sampler are always big, no small samplers if only 4 displayed) and option to expand them all 8 at once is now finished.

Is that what you had in mind?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 21, 2018

I think this is confusing. Sampler 5 jumps from being just right of the center to the bottom left when expanded. Whether it's
1 2 3 4
5 6 7 8
or
1 2 5 6
3 4 7 8
I think it's confusing to break what was 8 samplers on one row into multiple rows.

<ConfigKey>[Master],skin_settings</ConfigKey>
<BindProperty>visible</BindProperty>
</Connection>
</WidgetGroup>
Copy link
Member

Choose a reason for hiding this comment

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

This spacer is already in samplers_rack.xml, it can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<ConfigKey>[Master],skin_settings</ConfigKey>
<BindProperty>visible</BindProperty>
</Connection>
</WidgetGroup>
Copy link
Member

Choose a reason for hiding this comment

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

This spacer is already in samplers_rack.xml, it can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<WidgetGroup><Size>2f,1min</Size></WidgetGroup>

<!-- Don't know why, but this button has to be inverted..-->
<PushButton>
Copy link
Member

Choose a reason for hiding this comment

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

please keep an eye on the indentations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done


<!-- First row -->
<WidgetGroup>
<ObjectName>SamplerTextSmall</ObjectName>
Copy link
Member

Choose a reason for hiding this comment

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

here & below: please keep an eye on the indentations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<ConfigKey>[Master],skin_settings</ConfigKey>
<BindProperty>visible</BindProperty>
</Connection>
</WidgetGroup>
Copy link
Member

Choose a reason for hiding this comment

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

This spacer is already in samplers_rack.xml, it can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<attribute persist="true" config_key="[Microphone],show_microphone">0</attribute>
<attribute persist="true" config_key="[EffectRack1],show">0</attribute>
<attribute persist="true" config_key="[Samplers],show_samplers">0</attribute>
<attribute persist="true" config_key="[Master],show_4sampler">0</attribute>
Copy link
Member

@ronso0 ronso0 Oct 22, 2018

Choose a reason for hiding this comment

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

By "8 samplers are default" I meant to use a CO like [Skin],8_samplers and set the default to 1.
This is consistent with [..],show_4deck, [..],show_4effectunits & [..],8_hotcues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

</WidgetGroup>
<Template src="skin:skin_settings_button_2state.xml">
<SetVariable name="text">only 4 Samplers</SetVariable>
<SetVariable name="Setting">[Master],show_4sampler</SetVariable>
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned above: please use "8 Samplers" and [..],8_samplers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<SetVariable name="samplernum">3</SetVariable>
</Template>
<!-- Select between 5-8 big or 8 tiny -->
<WidgetStack currentpage="[Samplers],show_big">
Copy link
Member

@ronso0 ronso0 Oct 22, 2018

Choose a reason for hiding this comment

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

[Skin],expand_samplers or ..,samplers_expanded

Copy link
Member

Choose a reason for hiding this comment

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

please keep an eye on the indentations, and please make the comments match the actual widgets


<WidgetGroup><Size>10f,1min</Size></WidgetGroup>
<!--8 tiny Sampler -->
<WidgetGroup trigger="[Samplers],show_big5678_big_of_8" on_hide_select="0">
Copy link
Member

Choose a reason for hiding this comment

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

should be the same as above: [Skin],expand_samplers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

<ConfigKey>[Samplers],show_big</ConfigKey>
<ButtonState>LeftButton</ButtonState>
</Connection>
</PushButton>
Copy link
Member

Choose a reason for hiding this comment

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

IMO this button should be at the far right, like the FX expand buttons

@ronso0
Copy link
Member

ronso0 commented Oct 22, 2018

I think it's confusing to break what was 8 samplers on one row into multiple rows.

I think the grouping will be clear if we use a significant spacer in the center of the collapsed samplers, as well.


<WidgetGroup><Size>2f,1min</Size></WidgetGroup>

<!-- Don't know why, but this button has to be inverted..-->
Copy link
Member

Choose a reason for hiding this comment

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

see review in samplers_rack.xml for explanation

<SetVariable name="samplernum">3</SetVariable>
</Template>
<!-- Select between 5-8 big or 8 tiny -->
<WidgetStack currentpage="[Samplers],show_big">
Copy link
Member

Choose a reason for hiding this comment

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

items in a WidgetStack are 0-indexed.
Therefore, if [Samplers],show_big = 1 the stack shows the 2nd item (big samplers).
[Samplers],show_big = 0 the stack shows the 1st item (small samplers).
You can change the item order to match the not-inverted expand button.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@nopeppermint
Copy link
Contributor Author

@ronso0 thanks for the review, I will look into your points.

@ronso0
Copy link
Member

ronso0 commented Oct 22, 2018

I hope I don't request too much.. (thanks for your work so far :)
Big samplers 1-4 are used in two templates (so they are built twice) therefore they could be instantiated as Singletons in samplers_rack.xml

Also, please order the samplers like below and add a wider spacer in between small samplers 4 & 5, so we can check if the concept works.
1 2 5 6
3 4 7 8

@nopeppermint nopeppermint force-pushed the test2 branch 12 times, most recently from ee84eae to 60e3ec6 Compare November 1, 2018 17:22
@nopeppermint
Copy link
Contributor Author

This clears the commit history a bit..

better now?

@Be-ing
Copy link
Contributor

Be-ing commented Nov 1, 2018

Please use more descriptive commit messages than "fix". You can edit commit messages with git rebase -i

@nopeppermint
Copy link
Contributor Author

anything else? @ronso0 @Be-ing

@ronso0
Copy link
Member

ronso0 commented Nov 3, 2018

still LGTM.

The first 8 commits (everything before refactor layout, fix expand button, add 4 small samplers) contain a lot of back-and-forth changes. I think those can be squashed into one commit. But that's no big deal IMO, it's just for the sake of clarity.

nopeppermint and others added 5 commits November 3, 2018 20:38
first draw of small sampler, remove no longer used files and minor fix

add expand/collapse button, bpm, tooltip and smaller font

4 small or big sampler, or 8 tiny sampler with either the 4 left or 4 right big

remove 4 sampler option, adapt small sampler for two rows

remove no longer used buttons, add new sampler idea

remove spacers and change attribute names

fix tab/whitespace
LateNight: optimize SVGs, decrease Eject & Keylock icons
fixes scaling with high pixel density screens (the templates use
scalemode="STRETCH_ASPECT"). I do not know why setting
scalemode="STRETCH_ASPECT" for the crossfader assignment and
sampler expansion <Unpressed> elements does not work.
@nopeppermint
Copy link
Contributor Author

done, now we are down on 5 commits (from about 25 ...)

@Be-ing Be-ing merged commit da17055 into mixxxdj:2.2 Nov 3, 2018
@Be-ing Be-ing mentioned this pull request Nov 3, 2018
@rryan
Copy link
Member

rryan commented Nov 3, 2018

thanks @nopeppermint !

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.

5 participants