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

Added exponentialRampToValueAtTime feature #428

Closed
wants to merge 3 commits into from

Conversation

endurance21
Copy link
Collaborator

fixes
#427

@endurance21
Copy link
Collaborator Author

@therewasaguy
Please review the Pr

@endurance21
Copy link
Collaborator Author

I will be adding unit tests for this soon 😊

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

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

@endurance21 I have similar feedback to the comment on the new feature PR:

As far as new features go, a key decision from the recent p5 Contributors Conference is that "p5.js will not add any new features except those that increase access" (more here)

So let's keep that in mind as p5 contributors. New features add complexity and we should focus on making the existing features as stable and accessible as can be.

One way to make the library more accessible is to make the API more consistent.

This PR would add a feature to p5.Gain's amp method without adding it to other amp methods. This makes the API inconsistent (moreso than it already is!). I think the inconsistencies make the library less accessible and more confusing.

I think this is a good idea if it can be implemented in a consistent way. But first I would prioritize making the API consistent before adding new features.

@therewasaguy
Copy link
Member

@endurance21 how would you feel about adding this idea to the backlog for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants