Currently, the number of channels (and therefore the number of Plots) within a Plotter can change dynamically. The original reason to keep all the state in arrays in the Plotter was that it can be set once and then the values can change underneath.

a = Plotter.new;

a.plotColors = [Color.green, Color.blue, Color.red];

a.value = Env.perc(0.3, 0.7).discretize;
a.value = Env.perc([0.3, 0.4, 0.8], [0.7, 0.6, 0.2]).discretize;
a.value = [[1, 2], [18, 9]];

Because every Plot has only one channel, the clearest is to either:

1. keep the specifications (like plotColors) in Plotter, and refer to it from the Plot
2. or have a separate set of instance variables in Plot and copy the information at its index into the Plot

(2) is more efficient, (1) is easier to understand and maintain. If we don't run into efficiency problems, (2) might be the best (I am sorry if I gave a different recommendation earlier).

Sorry to drag on this, but I knew it would take some unwinding to get an idea of the scope of problem.

I think we should briefly get clear on where all the state is kept. Right now it is confusing (it might have been a bit confusing already before).

Yea... the state is smeared across both objects pretty well 😕 For the sake of background, I did a deep dive on how the state is managed, which I'll put below, but to keep this discussion on the practical issue of method/variable names...

In sclang, we often treat arrays the same as elements, and I can't remember where I've seen a case in which we keep both singular and plural forms spearately as methods.
I'd prefer if plotMode and plotModes were synonyms.
The same goes for plotColors. There is now an instance variable both in Plotter and in Plot.

This is helpful perspective! To clarify, you'd like to see if specs, domainSpecs, plotModes, and plotColors could all be removed and only the singular form is used?
I'll have a look at how far back the history goes with the plural state vars and methods to see if changing everything to singular would break anything.

State storage in Plot vs. Plotter

Here are some of the state variables that have some aspects of being shared between the two (omitting states which should clearly live in one instead of the other, like superpose in Plotter).

Further, all these parameters belong exclusively to Plot

• font, fontColor, gridColorX, gridColorY, backgroundColor, gridOnX, gridOnY, labelX, labelY

So if you want to set any of those, you need to access Plotter.plots[n], setting each one if superpose == false. IMO, because Plot doesn't seem to be intended to be accessed directly, it would be desirable to be able to change all these parameters through Plotter.

The original reason to keep all the state in arrays in the Plotter was that it can be set once and then the values can change underneath.

This seems sensible, but the state is also stored in Plot. Depending on the state of superpose, the data/value is either the same object as that in Plotter or a flopped copy:

p = 4.collect({15.collect({rrand(0,1.0)})}).plot;
p.plotColors_(4.collect{Color.rand}); p.refresh;

p.superpose = false;
p.value[0] === p.plots[0].value // true
p.superpose = true;
p.value[0] === p.plots[0].value // false

Storing the same data multiple ways also happens in Plotter

p.superpose = false;
p.data === p.value; // true
p.superpose = true;
p.data === p.value; // false

As an aside, I'd imagine this is pretty inefficient for large datasets.

Because every Plot has only one channel

This is also complicated by superpose:

p = 4.collect({15.collect({rrand(0,1.0)})}).plot;
p.plotColors_(4.collect{Color.rand}); p.refresh;

// observe Plotter's value wrt superpose
p.superpose = true;
p.value.shape
// -> [4, 15]
p.superpose = false;
p.value.shape
// -> [4, 15] // remains constant when superposed

// observe Plot's value wrt superpose
p.superpose = false;
p.plots 
// -> [ a Plot, a Plot, a Plot, a Plot ] // four plots
x = p.plots[0].value // value at first plot
x.shape 
// -> [15]  // first channel of data in the first plot

p.superpose = true;
p.plots 
// -> [ a Plot ] // one plots
x = p.plots[0].value // value at first plot
x.shape 
// -> [15, 4] // all data stored in EACH plot, and multichannel data flopped (therefor copied) when superposed

Because Plot is actually at times one channel, and at other times many channels, I would think that Plot should be a lot simpler and just do the work of drawing the data stored in Plotter, and request it's state from Plotter as well, based on its index. I think this would look like a combination of both 1. and 2. you suggested above. But that also means migrating all those state vars that exist solely in Plot to Plotter and having a consistent array storage for each of them. Better yet, if each Plot were truly always one channel and simply drawn in separate overlaid UserViews when superposed.

In any case, I'm not sure what the larger overhaul would look like exactly, but that seems beyond the scope of this PR.

Thank you very much indeed for this careful analysis. If I remember correctly, the state in the Plot is mainly meant as optimization, but this may not be required.

I'd suggest that you simplify this PR a little (no new plural forms for now, or only plural, whatever seems better), and then you could propose a refactoring as you have sketched above already. Would that be a good way to proceed? Alternatively, if you don't have the time to do this, you can also make an issue ticket with the proposed refactoring in this thread.

This is helpful perspective! To clarify, you'd like to see if specs, domainSpecs, plotModes, and plotColors could all be removed and only the singular form is used?

I would use singular terms everywhere, and only add plural ones for backward compatibility where they are plural now.

I'll have a look at how far back the history goes with the plural state vars and methods to see if changing everything to singular would break anything.

We can then see if we deprecate the plural forms, but I don't mind to let them in if commented. The plotter is just a tool, so we don't build up much secondary code that depends on it. At the same time, probably there are a lot of snippets around which may break for people.

I would use singular terms everywhere, and only add plural ones for backward compatibility where they are plural now.

OK, I've made these changes so that there are no plural method additions.

I've also modified plotColors to use the singular, because this still lives in 3.10.3 (introduced in #4082) and hasn't been released. That means then that this PR would also need to go out with 3.10.3 to make sure the plural plotColors doesn't go out. I'm not sure where we are on the 3.10.3 release stage, is this manageable?

I also updated docs to reflect these changes.

I'd suggest that you simplify this PR a little (no new plural forms for now, or only plural, whatever seems better), and then you could propose a refactoring as you have sketched above already. Would that be a good way to proceed? Alternatively, if you don't have the time to do this, you can also make an issue ticket with the proposed refactoring in this thread.

I think this is a good way forward. I'll put together an issue that documents the points to be addressed in a future refactor.

Thanks, Julian!

Also note that I left the arguments to both plotMode_ and plotColor_ as plural.

	plotColor_ { |colors|

	plotMode_ { |modes|

This seemed OK to me to suggest that the argument need not be a single mode/color. But I'm OK changing those arg names to be singular if you think it's better.

Hi @telephon, just checking in to see if you'd like any further changes with this PR. Thanks!

PLEASE DON'T MERGE YET

I think we need to wait for #4678 to go in, merge 3.10 into develop and then rebase this to resolve conflicts.

please rebase after #4678

OK, it's been rebased and I think the conflicts have been resolved properly.

@dyfer

Looks like this PR now includes sc-el updates (?). I'm not sure why/how that happens, submodules are still quite a mystery to me.

@brianlheim since you marked it as changes requested (to block merging before conflict resolution) I think it would be great if you gave it a green light. The only remaining question is whether anything needs to be done about sc-el...

edit: maybe this needs git submodule update --recursive?

Looks like this PR now includes sc-el updates (?).

Thanks for catching that @dyfer. I rewound and removed those sneaky submodule changes. It actually made the commit history more concise anyway.

AFAICT this is ready to be merged.

This PR has been reviewed before by @telephon and myself and was waiting to be rebased after #4678 got in, which has indeed happened. @brianlheim would you mind giving this a green light?

@dyfer sorry for the delay. i've simply dismissed my review, would prefer others to manage this PR. i don't have much interest in it and seems other people already know it better :)