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

testsuite: fix NodeProxy ar mapping test #5717

Merged
merged 2 commits into from
Feb 10, 2022
Merged

testsuite: fix NodeProxy ar mapping test #5717

merged 2 commits into from
Feb 10, 2022

Conversation

elgiano
Copy link
Contributor

@elgiano elgiano commented Feb 8, 2022

Purpose and Motivation

Fix what looks like a typo in TestNodeProxyBusMapping:test_audiorate_mapping.

I think control proxy for this test should be audio rate, for the mapping to be audio rate. Other than this, having it control rate with no numChannels specified makes it mixed down to mono by default, creating the discrepancy that makes this test impossible to pass. Initializing controlProxy as \audio fixes it.

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: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR label Feb 8, 2022
@dyfer
Copy link
Member

dyfer commented Feb 8, 2022 •
edited
Loading

Hi @elgiano
Could you also remove skipping for this test in our test suite? (and do the same for #5716?)

@dyfer dyfer mentioned this pull request Feb 8, 2022
Open
20 tasks
@elgiano elgiano force-pushed the topic/test_nodeproxymap branch from 145c157 to f90657d Compare February 8, 2022 18:11
@telephon
Copy link
Member

telephon commented Feb 8, 2022

Actually one thing: controlProxy is a misnomer, I think. Mayne you can rename it to inputProxy?

@elgiano elgiano force-pushed the topic/test_nodeproxymap branch from f90657d to ffafaa9 Compare February 8, 2022 19:22
@elgiano
Copy link
Contributor Author

elgiano commented Feb 8, 2022

Actually one thing: controlProxy is a misnomer, I think. Mayne you can rename it to inputProxy?

Sure! I also reduced the server's latency in order for those waits to be enough, since I noticed it was still failing for this reason.
Isn't there a more reliable way to sync for NodeProxies' set()?

@elgiano elgiano force-pushed the topic/test_nodeproxymap branch from ffafaa9 to 73983cf Compare February 8, 2022 19:48
@elgiano elgiano force-pushed the topic/test_nodeproxymap branch from 73983cf to dd009ec Compare February 8, 2022 19:50
@dyfer
Copy link
Member

dyfer commented Feb 8, 2022

Maybe this is a larger question, but wouldn't it be possible to set server latency to nil and use s.sync instead of wait times?

@elgiano
Copy link
Contributor Author

elgiano commented Feb 8, 2022 •
edited
Loading

sync doesn't work because NodeProxy doesn't send OSC to server immediately... for example wrapping it in makeBundle doesn't generate any bundle. I'm still investigating if there's any other way though.

Edit: and I can't find it... @telephon do you know anything we could do to avoid guessing a time to wait and use server.sync instead?

@telephon
Copy link
Member

telephon commented Feb 9, 2022 •
edited
Loading

sync doesn't work because NodeProxy doesn't send OSC to server immediately... for example wrapping it in makeBundle doesn't generate any bundle. I'm still investigating if there's any other way though.

That is strange, shouldn't be like this.

(
n = NodeProxy(s);
s.openBundle;
	n.source = { |freq=332| SinOsc.ar(freq) * 0.1 };
	s.sync;
	n.play;
	s.sync;
	n.set(\freq, 341);
s.addr.bundle.postcs;
s.closeBundle;
)

returns

-> [ 
[ syncFlag, nil, nil ], 
[ 21, 1146, 1, 1 ], 
[ 9, system_link_audio_1, 1147, 1, 1146, out, 0, in, 8, level, 1.0, fadeTime, 0.02, vol, 1.0 ], 
[ syncFlag, nil, nil ], 
[ 15, 1144, freq, 341 ] 
]

Right now, I don't understand why the synthDef sending is not included in the bundle.

@elgiano
Copy link
Contributor Author

elgiano commented Feb 9, 2022

I think it's because OSCBundle:doPrepare runs a Routine. This is going really far :) I'll put this info here but then let's open a new issue if we want to go further.

Replacing Routine.run with forkIfNeeded in SCClassLibrary/Common/Control/OSCBundle.sc at line 43, and wrapping your code in a routine, we get this bundle:

[ 
    [ 'syncFlag', [ 
        [ 5, Int8Array[ 83, 67, 103, 102, 0, 0, 0, 2, 0, 1, 23, 116, 101, 109, 112, 95, 95, 48, 45, 49, 49, 50, 51, 48, 53, 54, 56, 57, 50, 95, 49, 48, 50, 55, 0, 0, 0, 5, 0, 0, 0, 0, 63, -128, 0, 0, 64, 0, 0, 0, -62, -58, 0, 0, 64, 64, 0, 0, 0, 0, 0, 4, 67, -90, 0, 0, 63, -128, 0, 0, 60, -93, -41, 10, 0, 0, 0, 0, 0, 0, 0, 4, 4, 102, 114, 101, 113, 0, 0, 0, 0, 4, 103, 97, 116, 101, 0, 0, 0, 1, 8, 102, 97, 100, 101, 84, 105, 109, 101, 0, 0, 0, 2, 3, 111, 117, 116, 0, 0, 0, 3, 0, 0, 0, 8, 7, 67, 111, 110, 116, 114, 111, 108, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 1, 2, 68, 67, 2, 0, 0, 0, 1, 0, 0, 0, 1, 0, 0, -1, -1, -1, -1, 0, 0, 0, 0, 2, 7, 67, 111, 110, 116, 114, 111, 108, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 1, 1, 7, 67, 111, 110, 116, 114, 111, 108, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 2, 1, 6, 69, 110, 118, 71, 101, 110, 1, 0, 0, 0, 17, 0, 0, 0, 1, 0, 0, 0, 0, 0, 2, 0, 0, 0, 0, -1, -1, -1, -1, 0, 0, 0, 1, -1, -1, -1, -1, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 0, 0, -1, -1, -1, -1, 0, 0, 0, 2, -1, -1, -1, -1, 0, 0, 0, 0, -1, -1, -1, -1, 0, 0, 0, 2, -1, -1, -1, -1, 0, 0, 0, 1, -1, -1, -1, -1, 0, 0, 0, 3, -1, -1, -1, -1, 0, 0, 0, 1, -1, -1, -1, -1, 0, 0, 0, 1, -1, -1, -1, -1, 0, 0, 0, 4, -1, -1, -1, -1, 0, 0, 0, 0, -1, -1, -1, -1, 0, 0, 0, 0, -1, -1, -1, -1, 0, 0, 0, 1, -1, -1, -1, -1, 0, 0, 0, 4, -1, -1, -1, -1, 0, 0, 0, 0, 1, 12, 66, 105, 110, 97, 114, 121, 79, 112, 85, 71, 101, 110, 2, 0, 0, 0, 2, 0, 0, 0, 1, 0, 2, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 2, 7, 67, 111, 110, 116, 114, 111, 108, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 3, 0, 3, 79, 117, 116, 2, 0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 0, 0, 0 ] ], 
        [ 21, 1016, 1, 1 ] 
    ], nil ], 
    [ 9, 'temp__0-1123056892_1027', 1017, 1, 1016, 'out', 8, 'fadeTime', 0.02, 'i_out', 8 ], 
    [ 'syncFlag', nil, nil ], 
    [ 21, 1018, 1, 1 ], 
    [ 9, "system_link_audio_1", 1019, 1, 1018, "out", 0, "in", 8, "level", 1.0, "fadeTime", 0.02, "vol", 1.0 ], 
    [ 'syncFlag', nil, nil ], 
    [ 15, 1016, 'freq', 341 ] 
]

@elgiano
Copy link
Contributor Author

elgiano commented Feb 10, 2022

@dyfer @telephon Shall we merge this for now and eventually figure out other issues with NodeProxy separately?

@telephon telephon merged commit 42715a7 into supercollider:develop Feb 10, 2022
@elgiano elgiano deleted the topic/test_nodeproxymap branch February 10, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@telephon telephon telephon approved these changes

Assignees
No one assigned
Labels
comp: testing UnitTest class, refactors of existing tests, etc.; don't use if just adding tests as part of a PR
Projects
None yet
Milestone
No milestone
Development

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