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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

@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.

telephon added 2 commits June 18, 2018 14:51
@telephon
test: EnvGate should accept number as fadeTime
25c26c7
@telephon
class library: EnvGate uses direct start level
9965e42 
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
nhthn
nhthn reviewed Jun 20, 2018
View reviewed changes
testsuite/classlibrary/TestGraphBuilder.sc Outdated
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
mossheim
mossheim reviewed Jun 20, 2018
View reviewed changes
testsuite/classlibrary/TestGraphBuilder.sc Outdated
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.

#3801

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

@telephon telephon mentioned this pull request Jun 21, 2018
Closed
@nhthn nhthn removed this from the 3.10 milestone Jun 24, 2018
@telephon
Merge branch 'develop' into topic/fix/EnvGate-01
e73a44f 
# 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 •
edited
Loading

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)

nhthn
nhthn reviewed Jul 12, 2018
View reviewed changes
server/supernova/sc/sc_plugin_interface.cpp Outdated
@@ -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.

mossheim
mossheim suggested changes Jul 12, 2018
View reviewed changes
server/supernova/sc/sc_plugin_interface.cpp Outdated
@@ -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..

testsuite/classlibrary/TestGraphBuilder.sc Outdated
@@ -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.

mossheim
mossheim approved these changes Jul 22, 2018
View reviewed changes
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!

@adcxyz adcxyz merged commit 96db62c into supercollider:3.10 Jul 22, 2018
@mossheim mossheim mentioned this pull request Feb 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nhthn nhthn nhthn left review comments

@mossheim mossheim mossheim approved these changes

@muellmusik muellmusik Awaiting requested review from muellmusik

Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: class library SC class library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@telephon @mossheim @adcxyz @nhthn