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

sclang: Ugen: composeBinaryOp add missing ^ #5000

Merged
merged 1 commit into from
Jun 12, 2020
Merged

sclang: Ugen: composeBinaryOp add missing ^ #5000

merged 1 commit into from
Jun 12, 2020

Conversation

elgiano
Copy link
Contributor

@elgiano elgiano commented Jun 9, 2020 •
edited
Loading

Purpose and Motivation

UGen:composeBinaryOp checks if the other operand isValidUGenInput, and if not, calls otherOperand.performBinaryOpOnUgen(this) (pseudo). However, it doesn't return the latter's return value, making it ineffective.

This has been like that since the dawn of time (+18 years?), however, all implementations of performBinaryOpOnUgen seem to expect their value to be returned:

supercollider/SCClassLibrary/Common/Math/Complex.sc

Line 78 in f65a1ef

^aUGen.asComplex.perform(aSelector, this, adverb)

supercollider/SCClassLibrary/Common/Math/Polar.sc

Line 50 in f65a1ef

^Complex.new(

supercollider/SCClassLibrary/Common/Core/Object.sc

Line 683 in 9be3fe2

^this.performBinaryOpOnSomething(aSelector, thing, adverb)

EDIT: another impementation found in Quarks
https://github.com/supercollider-quarks/downloaded-quarks/blob/3b0d2f1e405587613e3e5f43103184f231552c42/MathLib/classes/SpherCoords/Spherical.sc#L115

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • This PR is ready for review

@elgiano elgiano added the comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library" label Jun 9, 2020
telephon
telephon approved these changes Jun 10, 2020
View reviewed changes
Copy link
Member

@telephon telephon left a comment

Choose a reason for hiding this comment

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

Quite amazing that we haven't noticed this over such a long time. Could you post a test case?

@elgiano
Copy link
Contributor Author

elgiano commented Jun 10, 2020 •
edited
Loading

Yes, I run into this with a custom class that defines isValidUGenInput to false. But we can do an example with Complex:

// THEORY
// before: dc * complex ignores complex
Complex(1,1) * DC.kr(1); // -> Complex( an OutputProxy, an OutputProxy )
DC.kr(1) * Complex(1,1); // -> an OutputProxy

// after: complex is not ignored anymore
Complex(1,1) * DC.kr(1); // -> Complex( an OutputProxy, an OutputProxy )
DC.kr(1) * Complex(1,1); // -> Complex( an OutputProxy, an OutputProxy )

// PRACTICE
// before
{(Complex(10,10) * DC.kr(1)).real.poll}.play // -> 10
{(DC.kr(1) * Complex(10,10)).real.poll}.play // -> 1 :(

// after
{(Complex(10,10) * DC.kr(1)).real.poll}.play // -> 10
{(DC.kr(1) * Complex(10,10)).real.poll}.play // -> 10 :)

mossheim
mossheim suggested changes Jun 11, 2020
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! can you please add a test?

@elgiano elgiano force-pushed the topic/ugen-composeBinaryOp branch from cd2d0f8 to e57c8b5 Compare June 12, 2020 11:40
@elgiano
Copy link
Contributor Author

elgiano commented Jun 12, 2020

@brianlheim thanks for checking this out!
I added a test in TestComplex, as it would be hard to test this feature on UGen without an operand. I opted to not boot a server for this test, I think it's enough to test the return value's class: I think it makes sense to test that (UGenComplex) returns the same class as (ComplexUGen).

Please let me know if you think it's ok enough, or if it would be appropriate to test the op's return values, or anything else really! :)

mossheim
mossheim approved these changes Jun 12, 2020
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!

@mossheim mossheim merged commit fba43fa into supercollider:develop Jun 12, 2020
@elgiano
Copy link
Contributor Author

elgiano commented Jun 12, 2020

thank you!

@elgiano elgiano deleted the topic/ugen-composeBinaryOp branch June 12, 2020 17:10
@patrickdupuis patrickdupuis mentioned this pull request Jul 6, 2020
Merged
4 tasks
mossheim added a commit that referenced this pull request Jul 7, 2020
@mossheim
Merge pull request #5058 from patrickdupuis/cherry-pick5000
6b77ef0 
Cherry-pick #5000 into 3.11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

@telephon telephon telephon approved these changes

Assignees
No one assigned
Labels
comp: sclang sclang C++ implementation (primitives, etc.). for changes to class lib use "comp: class library"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elgiano @telephon @mossheim