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

Fix EnvGate issues #3797

Merged
merged 8 commits into from Jul 22, 2018
Merged

Fix EnvGate issues #3797

merged 8 commits into from Jul 22, 2018

Conversation

telephon
Copy link
Member

@telephon telephon commented Jun 18, 2018

@telephon
class library: EnvGate uses direct start level

Instead of relying on an indirect clue, we follow the documentation for
i_level, interpreting it as initial level. This fixes #3768, #3795.

testing: all uses of EnvGate that used the i_level parameter to scale
the resulting envelope. In class library, there is no such use.

this is a change to the implemented, not however to the documented API.

Instead of relying on an indirect clue, we follow the documentation for
`i_level`, interpreting it as initial level. This fixes supercollider#3768,  supercollider#3795.

# testing: all uses of EnvGate that used the i_level parameter to scale
the resulting envelope. In class library, there is no such use.

this is a change to the implemented, not however to the documented API.
@telephon telephon requested a review from muellmusik June 19, 2018 19:57
try {
{ EnvGate(fadeTime:1) }.asSynthDef
} { |e| error = e };
error.postln;
Copy link
Contributor

Choose a reason for hiding this comment

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

extraneous postln

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, fixed.

@nhthn nhthn added the comp: class library SC class library label Jun 20, 2018
@nhthn nhthn added this to the 3.10 milestone Jun 20, 2018
try {
{ EnvGate(fadeTime:1) }.asSynthDef
} { |e| error = e };
this.assert(error.isException.not, "number should be a valid EnvGate fadeTime")
Copy link
Contributor

Choose a reason for hiding this comment

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

We need an assertThrows

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll implement one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

you are awesome

@nhthn nhthn removed this from the 3.10 milestone Jun 24, 2018
# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@telephon telephon added the bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. label Jul 10, 2018
@telephon
Copy link
Member Author

Actually, I'd prefer to merge this into 3.10 in the current form and improve the test later, once the assertException pull request has been merged.

@mossheim
Copy link
Contributor

mossheim commented Jul 10, 2018

assertException was merged already though, wasn't it? so you could just rebase this and add the test

@telephon
Copy link
Member Author

ah, I'm dizzy. thanks.

@telephon
Copy link
Member Author

@brianlheim ok, done!

@mossheim
Copy link
Contributor

Can you please rebase it on current develop? There are merge conflicts (I think this is Githib's issue actually)

@@ -987,6 +987,7 @@ void sc_plugin_interface::buffer_alloc_read_channels(uint32_t index, const char
uint32_t frames, uint32_t channel_count,
const uint32_t * channel_data)
{

Copy link
Contributor

Choose a reason for hiding this comment

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

:/

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha where did this come from? Weird..

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe from fixing the merge conflicts in github.

@@ -987,6 +987,7 @@ void sc_plugin_interface::buffer_alloc_read_channels(uint32_t index, const char
uint32_t frames, uint32_t channel_count,
const uint32_t * channel_data)
{

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha where did this come from? Weird..

@@ -0,0 +1,11 @@

TestEnvGate : UnitTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be named TestEnvGate; if a more specific name is desired you could use TestEnvGate_GraphBuilding

Copy link
Member Author

Choose a reason for hiding this comment

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

done, thanks!

@adcxyz
Copy link
Contributor

adcxyz commented Jul 22, 2018

anything still missing here?
maybe remove the extra blank line ;-), and then its ready?

@telephon telephon changed the base branch from develop to 3.10 July 22, 2018 18:26
@telephon
Copy link
Member Author

ready to go, fixes two bugs.

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants