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

class library: plot relative to target synth #3088

Merged
merged 10 commits into from May 12, 2018

Conversation

telephon
Copy link
Member

@telephon telephon commented Aug 1, 2017

Sometimes, one needs to plot values from a synth that depends on node
order. E.g. a synth that reads from a bus. This patch allows us to
determine the point at which the plotting synth is inserted in the node
order. It is inserted after the target node. By default, this is the
default group of the default server. This follows the asTarget
interface, as documented in scdoc.

Sometimes, one needs to plot values from a synth that depends on node
order. E.g. a synth that reads from a bus. This patch allows us to
determine the point at which the plotting synth is inserted in the node
order. It is inserted _after_ the target node. By default, this is the
default group of the default server. This follows the `asTarget`
interface, as documented in scdoc.
@joshpar
Copy link
Member

joshpar commented Aug 1, 2017 via email

@telephon
Copy link
Member Author

telephon commented Aug 1, 2017

Ah definitely. I don't have the time now, but it should be easy.

Also, I'd love to have a node-based server meter that lets you see all the signals of all the nodes and busses. But that's for later :)

@telephon
Copy link
Member Author

telephon commented Aug 1, 2017

(for scope, it is in BusScopeSynth::play).

@mossheim mossheim added comp: class library SC class library comp: Qt GUI sclang Qt components -- for IDE tickets, use "env: SCIDE" instead labels Aug 2, 2017
@adcxyz
Copy link
Contributor

adcxyz commented Aug 5, 2017

really nice, thanks!

Also, I'd love to have a node-based server meter that lets you see all the signals of all the nodes and busses. But that's for later :)

Maybe ProxyMeter (in JITLibExtensions) could be extended/adapted easily for this ?

@adcxyz adcxyz requested review from adcxyz and removed request for adcxyz August 5, 2017 17:47
argument::duration
The duration of the function to plot in seconds. The default is 0.01.
argument::server
The Server on which to calculate the plot. This must be running on your local machine, but does not need to be the internal server. If nil the default server will be used.
The Server on which to calculate the plot. This must be running on your local machine, but does not need to be the internal server. If nil the default server will be used. This argument can also be a group or synth, it then is used as a target (see link::Reference/asTarget::):
Copy link
Contributor

Choose a reason for hiding this comment

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

can this argument name also change to target?

Copy link
Member Author

Choose a reason for hiding this comment

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

if we all agree yes, please. I just tried not to change the interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will make the current PR an API change. Do we want this? We could also do this change later, and have this one in 3.9.

Copy link
Contributor

@adcxyz adcxyz Sep 20, 2017

Choose a reason for hiding this comment

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

I vote for change to argument: target

  1. it expresses the power and flexibility of the method very clearly
    1.a by comparison, the arg 'server' makes it confusing that it can also be any target,
    and makes it necessary to explain in the help file - which may not be seen often.
  2. Function.plot is a utility for checking what the signal resulting from a short synthesis snippet looks like; my guess is that in >99% of the cases it will run silently on the default server, and in >99% of those uses no one specifies a server by keyword.
    Building any larger projects by relying on code like
    ~myplotter = { ... }.plot(server: ~mySpecialServer);
    seems really unlikely, and would IMHO not be advisable style.

2c adc

This applies to the methods:

- Function:plot
- Function: loadToFloatArray
- Function:asBuffer
@nhthn nhthn changed the base branch from master to develop September 24, 2017 07:13
Copy link
Contributor

@muellmusik muellmusik left a comment

Choose a reason for hiding this comment

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

Generally I like the idea. A few comments.

argument::server
The Server on which to calculate the plot. This must be running on your local machine, but does not need to be the internal server. If nil the default server will be used.
argument::target
The target node on which to calculate the plot. See link::Reference/asTarget::. If it is a link::Classes/Server::, it must be running on your local machine, but does not need to be the internal server (see link::Classes/Buffer#-loadToFloatArray:: for details).
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the implementation really require a local server? Why? getToFloatArray should work or?

Also, it would be useful to explain what's happening (addAfter) with target and why. With Function:plot presumably target only matters if you are reading from a bus. I find 'on which to calculate the plot' a bit enigmatic for newbies.

Copy link
Contributor

Choose a reason for hiding this comment

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

It uses loadToFloatArray, presumably for speed. In general, the class library seems to avoid getToFloatArray; I'm not sure why.

For the default 100ms plot duration, get... rather than load... will not be any slower -- might even be faster. For longer plots, load... is much faster.

Copy link
Member Author

@telephon telephon Oct 4, 2017

Choose a reason for hiding this comment

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

I think getToFloatArray was added later, and everyone, including me, is copying the old one.

Copy link
Contributor

Choose a reason for hiding this comment

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

IAC plotting on remote servers seems desirable.

Copy link
Member Author

@telephon telephon Oct 4, 2017

Choose a reason for hiding this comment

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

Also, I think the getToFloatArray looks more complicated and insecure with the wait and timeout arguments. Also the argument names index and count don't match the ones of loadToFloatArray.

That might be the reason. Maybe we need the same thing as we have for SynthDef, something that tries to send first and if that fails, tries to load? All packed under a clean interface.

argument::bounds
An instance of Rect or Point indicating the bounds of the plot window.
argument::minval
the minimum value in the plot. Defaults to -1.0.
argument::maxval
the maximum value in the plot. Defaults to 1.0.
argument:: separately
For multi channel signals, set whether to use separate value display ranges or not.
a window to place the plot in. If nil, one will be created for you.
If set to code::true::, for multichannel signals use separate value display ranges.
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these specified?

@telephon
Copy link
Member Author

telephon commented Oct 4, 2017

OK, here are some improvements - thanks for your ideas!

@telephon
Copy link
Member Author

telephon commented Nov 2, 2017

ping

Copy link
Contributor

@adcxyz adcxyz left a comment

Choose a reason for hiding this comment

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

little glitch: in Function.schelp lines 455 and 457, minimum and maximum are swapped between argument names and description lines. everything else looks good!

@mossheim mossheim added this to the 3.10 milestone Feb 4, 2018
@@ -769,14 +769,13 @@ Plotter {


+ Function {
plot { |duration = 0.01, server, bounds, minval, maxval, separately = false|

plot { |duration = 0.01, target, bounds, minval, maxval, separately = false|
Copy link
Contributor

Choose a reason for hiding this comment

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

changing function parameter names is a breaking change...

@@ -311,15 +313,24 @@ Function : AbstractFunction {
^buffer
}

loadToFloatArray { |duration = 0.01, server, action|
this.asBuffer(duration, server, { |buffer|
loadToFloatArray { |duration = 0.01, target, action|
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, breaking change

asBuffer { |duration = 0.01, server, action, fadeTime = (0)|
var buffer, def, synth, name, numChannels, rate;
server = server ? Server.default;
asBuffer { |duration = 0.01, target, action, fadeTime = (0)|
Copy link
Contributor

Choose a reason for hiding this comment

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

also breaking change

@mossheim
Copy link
Contributor

mossheim commented Feb 4, 2018

I would like to see this go through, but I think the general policy we've been following is not to change parameter names of methods because it potentially breaks user code. I don't know what to do in this case, because it makes semantic sense to change it.

@telephon
Copy link
Member Author

telephon commented Feb 4, 2018

I'd suggest the following: for now, I don't change the names, but when we make the next version step from 3.9 to 3.10, we do the breaking change. I could open an issue with that milestone, so we don't forget.

We could use that as an occasion to fix as many similar cases as possible.

@mossheim
Copy link
Contributor

mossheim commented Feb 4, 2018

I'd suggest the following: for now, I don't change the names, but when we make the next version step from 3.9 to 3.10, we do the breaking change.

This is already milestoned as 3.10... should it not be?

We could use that as an occasion to fix as many similar cases as possible.

If the policy is that we can always make breaking changes in a minor release, I don't see why we would need (or want) to group them.

@telephon
Copy link
Member Author

telephon commented Feb 4, 2018

In the past, such smaller breaking changes have often happened in minor releases. The major releases (sc1, sc2, sc3) were different: they basically broke everything.

If we want to change this, we need to speed up with major releases, otherwise we get stuck.

@telephon
Copy link
Member Author

what should we do? Merge it to develop as it is now?

@telephon
Copy link
Member Author

Is that ok to go into develop?

@telephon telephon merged commit eabd21d into supercollider:develop May 12, 2018
@nhthn
Copy link
Contributor

nhthn commented Jul 13, 2018

i'm a little dissatisfied with this loadToFloatArray/getToFloatArray API, one only working for local servers and one working for all servers but unreliably. i don't really dig the names either, the differences between them is not obvious.

would it make sense for Function:plot to call loadToFloatArray if the server is local and getToFloatArray if the server is remote?

if that's the case, could the get/load dichotomy be abstracted in the Buffer class? and also, let's take this opportunity to rename these methods so it's easier to tell what they're doing.

  • Buffer:getContentsWithSoundFile uses b_write.
  • Buffer:getContentsOverOSC uses repeated calls to /b_getn.
  • Buffer:getContents, which calls Buffer:getContentsWithSoundFile if the server is local and Buffer:getContentsOverOSC otherwise

so just by calling Buffer:getContents, the best retrieval method is selected for you.

@telephon
Copy link
Member Author

@snappizz could you please open a new issue for that? I agree that the names are not so good, but they have been around for decades and this is something separate from the current glitch that needs to be fixed.

@telephon
Copy link
Member Author

would it make sense for Function:plot to call loadToFloatArray if the server is local and getToFloatArray if the server is remote?

possible. As a first step, getToFloatArray should work in plot, then I'd try and abstract this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change comp: class library SC class library comp: Qt GUI sclang Qt components -- for IDE tickets, use "env: SCIDE" instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants