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

Distort0r: new parameters - mode & velocity #1

Merged
merged 1 commit into from
May 10, 2016
Merged

Conversation

d-j-a-y
Copy link
Contributor

@d-j-a-y d-j-a-y commented May 5, 2016

  • mode (bool) p3 : aim to select from 'time' (initial behavior) to speed based (controlled by p4 parameter) of the plasma signal
  • regarding mode, velocity parameter (p4) aim to control speed of the plasma signal

case 2:
info->name = "Mode";
info->type = F0R_PARAM_BOOL;
info->explanation = "\'Time Based\' or \'Adjustable Velocity\'";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, Mode with this explanation does not make it clear which value determines which mode. The quickest fix is to improve the explanation. Alternatively, Mode could be a string param that takes "time" or "velocity" as values, or it could be renamed to "Use Velocity."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mode with this explanation does not make it clear...

Firstly, i thought to do a "Use Velocity.", then i make a choice (wrong one), thinking of future, when more Mode could be implemented.... but KISS is the way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is good to think about the possibility of more modes in the future. In
that case, it would make sense to change param type to string, and use
keywords as described in the explanation. Thank you for the quick update!

On Mon, May 9, 2016 at 1:16 AM d-j-a-y notifications@github.com wrote:

In src/filter/distort0r/distort0r.c
#1 (comment):

@@ -66,6 +69,16 @@ void f0r_get_param_info(f0r_param_info_t* info, int param_index)
info->type = F0R_PARAM_DOUBLE;
info->explanation = "The frequency of the plasma signal";
break;

  • case 2:
  •  info->name = "Mode";
    
  •  info->type = F0R_PARAM_BOOL;
    
  •  info->explanation = "\'Time Based\' or \'Adjustable Velocity\'";
    

Mode with this explanation does not make it clear...

Firstly, i thought to do a "Use Velocity.", then i make a choice (wrong
one), thinking of future, when more Mode could be implemented.... but KISS
is the way.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
https://github.com/dyne/frei0r/pull/1/files/e67d2fc566c661fce055cbccce0e7b1f838752e2#r62462621

@ddennedy
Copy link
Collaborator

ddennedy commented May 8, 2016

I am fine with it once the minor complaints are addressed. Please do amend your commit instead of making new "fix" commits and then force push to your repo (I doubt anyone will be affected).

* mode (bool) p3 : aim to select from 'time' (initial behavior) to speed based (controlled by p4 parameter) of the plasma signal
* regarding mode, velocity parameter (p4) aim to control speed of the plasma signal
@ddennedy ddennedy merged commit b63f5db into dyne:master May 10, 2016
ddennedy pushed a commit that referenced this pull request Oct 25, 2019
Correction for cylindrical hue term distance
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.

None yet

2 participants