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

Delay Plugin Redesign #3120

Merged
merged 2 commits into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
@RebeccaDeField
Contributor

RebeccaDeField commented Nov 14, 2016

In this pull request I have:

  • Redesigned the plugin
  • Renamed the knobs for consistency
  • Added a grid
  • Changed the size of the plugin
  • Fixed alignment

Screenshot

2

@Umcaruje @BaraMGB

Related: #2831

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 14, 2016

Member

Thanks @RebeccaDeField .

In fear of sounding like a broken record... the contrast between the canvas and the rest of the UI is too low and could benefit from an increase in contrast.

Member

tresf commented Nov 14, 2016

Thanks @RebeccaDeField .

In fear of sounding like a broken record... the contrast between the canvas and the rest of the UI is too low and could benefit from an increase in contrast.

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 14, 2016

Contributor

@tresf Actually, we're on the same page right now because upon reviewing my PR I already started to modify my design with the same concern. I have am working on an idea that I believe will solve the problem.

Contributor

RebeccaDeField commented Nov 14, 2016

@tresf Actually, we're on the same page right now because upon reviewing my PR I already started to modify my design with the same concern. I have am working on an idea that I believe will solve the problem.

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 14, 2016

Contributor

@tresf What do you think about this?
shot-2016-11-13_19-50-36

Contributor

RebeccaDeField commented Nov 14, 2016

@tresf What do you think about this?
shot-2016-11-13_19-50-36

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 14, 2016

Member

@RebeccaDeField it certainly helps the slider on the right although the canvas is still hard to distinguish. Could this perhaps benefit from a grid pattern similar to some of the other plugins?

Member

tresf commented Nov 14, 2016

@RebeccaDeField it certainly helps the slider on the right although the canvas is still hard to distinguish. Could this perhaps benefit from a grid pattern similar to some of the other plugins?

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 14, 2016

Contributor

@tresf Do you think that the plugin would benefit from a grid from a usability standpoint? If so, I think we should definitely add a grid.

If it's just a matter of the distinguishing where the edges of the canvas are, maybe we can use a more apparent border to accomplish this?

Contributor

RebeccaDeField commented Nov 14, 2016

@tresf Do you think that the plugin would benefit from a grid from a usability standpoint? If so, I think we should definitely add a grid.

If it's just a matter of the distinguishing where the edges of the canvas are, maybe we can use a more apparent border to accomplish this?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 14, 2016

Member

Do you think that the plugin would benefit from a grid from a usability standpoint? If so, I think we should definitely add a grid.

Yes, I think so. Perhaps even long-term swap the Y axis and make the mouse responsive outside of the canvas area.

Member

tresf commented Nov 14, 2016

Do you think that the plugin would benefit from a grid from a usability standpoint? If so, I think we should definitely add a grid.

Yes, I think so. Perhaps even long-term swap the Y axis and make the mouse responsive outside of the canvas area.

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 14, 2016

Contributor

Yes, I think so.

@tresf Then I will make it so number 1. 😏 What kind of spacing do you think the grid should have? Would this grid benefit from major and minor lines, or do you think we should stick to something more simple?

Contributor

RebeccaDeField commented Nov 14, 2016

Yes, I think so.

@tresf Then I will make it so number 1. 😏 What kind of spacing do you think the grid should have? Would this grid benefit from major and minor lines, or do you think we should stick to something more simple?

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 14, 2016

Member

What kind of spacing do you think

Since one measurement is 0 - 1 and the other is 0 - 5, a 10 x 10 grid perhaps? Major lines in the center?

Member

tresf commented Nov 14, 2016

What kind of spacing do you think

Since one measurement is 0 - 1 and the other is 0 - 5, a 10 x 10 grid perhaps? Major lines in the center?

@RebeccaDeField RebeccaDeField changed the title from Delay Plugin Redesign to [WIP] Delay Plugin Redesign Nov 14, 2016

@RebeccaDeField RebeccaDeField referenced this pull request Nov 15, 2016

Closed

Redesign The Effects Plugins #2831

14 of 14 tasks complete
@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 20, 2016

Contributor

@tresf Your thoughts on this?

2

Contributor

RebeccaDeField commented Nov 20, 2016

@tresf Your thoughts on this?

2

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Nov 20, 2016

Member

@RebeccaDeField that looks great.

Member

tresf commented Nov 20, 2016

@RebeccaDeField that looks great.

@RebeccaDeField RebeccaDeField changed the title from [WIP] Delay Plugin Redesign to Delay Plugin Redesign Nov 21, 2016

@RebeccaDeField RebeccaDeField added gui and removed in progress labels Nov 21, 2016

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Nov 22, 2016

Member

I like this, though I find that 5 seconds worth of delay is quite useless with that XY pad, especially cause it's linear. But that's talk for another issue.

@RebeccaDeField Please change REGEN to the appropriate abbreviation we chose for Feedback on previous plugins, as that's what that is. Also, the Rate and the Amount are related to a LFO, so I wonder if we should indicate that visually somehow. Otherwise, top notch work!

P.S. I couldn't help but notice that that fader is rendering as linear, it would be good to set it to render in a dbFS scale

Member

Umcaruje commented Nov 22, 2016

I like this, though I find that 5 seconds worth of delay is quite useless with that XY pad, especially cause it's linear. But that's talk for another issue.

@RebeccaDeField Please change REGEN to the appropriate abbreviation we chose for Feedback on previous plugins, as that's what that is. Also, the Rate and the Amount are related to a LFO, so I wonder if we should indicate that visually somehow. Otherwise, top notch work!

P.S. I couldn't help but notice that that fader is rendering as linear, it would be good to set it to render in a dbFS scale

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Nov 22, 2016

Contributor

Please change REGEN to the appropriate abbreviation we chose for Feedback on previous plugins, as that's what that is.

👍

Also, the Rate and the Amount are related to a LFO, so I wonder if we should indicate that visually somehow.

Maybe change the labels to LFO AMNT and LFO RATE, or did you have something else in mind?

It would be good to set it to render in a dbFS scale

Sorry, but I don't know how to set this. A few pointers?

Thank you :)

Contributor

RebeccaDeField commented Nov 22, 2016

Please change REGEN to the appropriate abbreviation we chose for Feedback on previous plugins, as that's what that is.

👍

Also, the Rate and the Amount are related to a LFO, so I wonder if we should indicate that visually somehow.

Maybe change the labels to LFO AMNT and LFO RATE, or did you have something else in mind?

It would be good to set it to render in a dbFS scale

Sorry, but I don't know how to set this. A few pointers?

Thank you :)

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Nov 22, 2016

Member

Sorry, but I don't know how to set this. A few pointers?

Should be as simple to as adding a call to make the faders render in dbfs like

outFader->setLevelsDisplayedInDBFS();

here: https://github.com/LMMS/lmms/blob/master/plugins/Delay/DelayControlsDialog.cpp#L79

This is how it's done for the FX mixer: https://github.com/LMMS/lmms/blob/master/src/gui/FxMixerView.cpp#L284-L286

Maybe change the labels to LFO AMNT and LFO RATE, or did you have something else in mind?

Sure, if they end up fitting, otherwise a line and the text LFO over the knobs could also work.

Member

Umcaruje commented Nov 22, 2016

Sorry, but I don't know how to set this. A few pointers?

Should be as simple to as adding a call to make the faders render in dbfs like

outFader->setLevelsDisplayedInDBFS();

here: https://github.com/LMMS/lmms/blob/master/plugins/Delay/DelayControlsDialog.cpp#L79

This is how it's done for the FX mixer: https://github.com/LMMS/lmms/blob/master/src/gui/FxMixerView.cpp#L284-L286

Maybe change the labels to LFO AMNT and LFO RATE, or did you have something else in mind?

Sure, if they end up fitting, otherwise a line and the text LFO over the knobs could also work.

@RebeccaDeField

This comment has been minimized.

Show comment
Hide comment
@RebeccaDeField

RebeccaDeField Jan 19, 2017

Contributor

This has been updated then tested by @BaraMGB and myself. I'm merging. 👍

Contributor

RebeccaDeField commented Jan 19, 2017

This has been updated then tested by @BaraMGB and myself. I'm merging. 👍

@RebeccaDeField RebeccaDeField merged commit e56d318 into LMMS:master Jan 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment