Quefumas / gensound

Pythonic audio processing and generation framework
Apache License 2.0
80 stars 6 forks source link

Curves cannot take Curves as input #23

Open gregoiremenguy opened 2 years ago

gregoiremenguy commented 2 years ago

Hi,

I am playing with Curve but I observed that they cannot take as input another Curve object. I think it could be really useful. For example, if I want to create a tremolo effect, I can use a SineCurve to modulate the frequency of the signal. However, if I want the tremolo speed to vary over time, it is not possible as the frequency input of SineCurve needs to be a float.

Here is a solution I found. In SineCurve, the integral method could be replaced by something like:

def integral(self, sample_rate):
        if isinstance(self.frequency, Curve):
            freq = self.frequency.flatten(sample_rate)
        else:
            freq = self.frequency

        # sin (x-pi/2) + 1
        return self.depth*np.sin(2*np.pi*freq*self.sample_times(sample_rate, inclusive=False) - np.pi/2) + self.depth + self.baseline*self.sample_times(sample_rate,inclusive=False)

Note that I had to replace the option inclusive=True by inclusive=False (don't know why it was set to True).

Maybe it should be done also in the flatten function.

Quefumas commented 2 years ago

Hello!

You touched on something important here, so this is going to be long. TLDR: you are right, but this is part of a much larger issue.

Even though it was just an example, let me first address the practical question:

And now, to address the actual question, and the elephant in the room: what you are saying is 100% correct; except, we should go much further than just this change in the code. The way I see it for quite some time now, there shouldn't be any essential difference between Curve and Signal, and the two should perhaps even be merged together. Then, any frequency parameter anywhere should be able to accept any such signal of any kind, and likewise for any temporal parameter of any transform. Also, we could apply transforms to curves (this is useful to prevent sharp jumps in frequency, for example). This has been brewing in my head for a long time, but I haven't decided yet on the best way to approach it.

It's not a technological challenge, rather a design challenge. I wonder what would be the best way to reconcile the features of these two kinds of signals. Namely, should curves have a realise method, rather than flatten? Should Curve inherit from Signal? Or maybe the other way round? Should they have a common superclass? This is one of the top issues that need to be resolved in the future, but I'm still not confident about the best way to approach it.

gregoiremenguy commented 2 years ago

Hi @Quefumas

I think the effect you are describing is more commonly referred to as Vibrato rather than Tremolo, although I may be wrong. If I recall, Tremolo typically refers to amplitude modulation, which can be currently performed by feeding a SineCurve to an Amplitude Transform, as (very briefly) mentioned in the wiki section on automation.

You are right,I was talking about vibrato.

To perform vibrato, that is, modulating the frequency by an oscillator pattern, there already exists a Vibrato transform, which actually does receive curves. Would it work for what you were suggesting?

Yes it works well. Still, I think that the need to create transforms each time we need to give a Curve object as inputs to a Curve (as I was initialy doing) is not that practical especially to test new effects.

About the inclusive argument, unfortunately I didn't get to document it anywhere yet; when it is set to True it includes the 'end point' of the sample times, that is, the time where the last sample would end (rather than begin). This means it returns an array larger by 1 than the actual signal/curve. This is required sometimes to ensure smooth integration between adjacent curves. Seeing that someone other than myself actually needed it, this gives me some more motivation to better document these features of the code.

Ok, however such feature cause some problems when you put Curve object as frequency input. Indeed, numpy will not be able to handle the two shapes. For example, in the code I gave in my first message, freq has shape (size,) but the returned array (if incluse=True) has shape (size+1,). So maybe the solution would to add to each flatten function an inclusive option. What do you think ?

And now, to address the actual question, and the elephant in the room: what you are saying is 100% correct; except, we should go much further than just this change in the code. The way I see it for quite some time now, there shouldn't be any essential difference between Curve and Signal, and the two should perhaps even be merged together. Then, any frequency parameter anywhere should be able to accept any such signal of any kind, and likewise for any temporal parameter of any transform. Also, we could apply transforms to curves (this is useful to prevent sharp jumps in frequency, for example). This has been brewing in my head for a long time, but I haven't decided yet on the best way to approach it.

I totally agree. IMHO, it would make the design of gensound a lot closer to what is done with analogic synthesizers (afaik). If gensound merges Signals and Curves, it will give more freedom to the user (one could combine signals as wanted). In such conditions, a Transform would be a way to save interresting effects and would not be necessary to test new ones, e.g. the Vibrato effect must currently be implemented as a Transform forcing the user to manipulate Audio objects (which are harder to manipulate than Signal objects).

It's not a technological challenge, rather a design challenge. I wonder what would be the best way to reconcile the features of these two kinds of signals. Namely, should curves have a realise method, rather than flatten? Should Curve inherit from Signal? Or maybe the other way round? Should they have a common superclass? This is one of the top issues that need to be resolved in the future, but I'm still not confident about the best way to approach it.

I think that Curves could be removed entirely. In my understanding, the Curve.flatten function could be replaced by the Signal.realise function (however, I still don't understand the need of Curve.integral). Moreover, we could create a class FunctionSignal (inherinting Signal) to let the user give its own functions (as done in the Curve class). What do you think ?

Quefumas commented 2 years ago

First, some clarifications for future reference:

Next, I agree that we can eventually merge Curve.flatten into Signal.realise. Perhaps the best approach would be to set up a separate branch and gradually figure out everything that needs to be adapted. Here are some of the smaller tasks that should be done, including some concerns about this (some quite insignificant), so we can ensure they all get addressed eventually:

  1. Ensuring all Signals implement integral somehow (though they can all inherit the naive implementation currently available). In some cases we should use mathematics to compute it faster.
  2. Likewise for endpoint(). This function is supposed to return, in a sense, what value the curve would achieve had it been one sample longer. Example: if I multiply amplitude by a line curve, going up from 0 to 1 within 1000 samples, we would actually expect the value 1 to be achieved at the end of the final sample. This means that flatten never actually achieves the value 1. This is not necessarily a problem, but when I use this curve to control the amplitude of a signal longer than 1000 samples, I would expect the 1001th and every subsequent sample to be multiplied by 1 rather than 0.999. So Gensound uses endpoint to figure out what the line was supposed to reach, and continue it to the end of the affected signal. Also, I think this may be needed to ensure smooth changes when controlling frequency.
  3. An issue relating to this example: expressions of type Signal | float are interpreted as appending silence to a signal, while Curve | float are interpreted as extending the final value of the Curve object for a certain time span. It needs to be decided how to reconcile these two behaviours, as each of them makes sense in the individual contexts of the two classes.
  4. SineCurve has a baseline argument, which is desirable for control signals (curves). I guess if we combine it with Sine we would have to make this argument optional, as it doesn't make much sense for usual audio sine waves.
  5. Should we take more care about the types of signals that are used as control signals? This change will make it very easy to feed a frequency that may sometimes become negative, and I'm not sure such a bug would be easy for the user to solve on their own.
  6. Also the duration property is implemented for Curves, and actually it is not entirely straightforward to implement for all kinds of Signals (this is related to an important feature not yet available; in some cases it is useful for a signal to have a shorter theoretical duration than actual duration in practice).
  7. Related future issue to think about: ideally, we would like to be able to omit the duration argument in some cases. Currently, if I want to control a sine's frequency using a SineCurve, I have to supply the duration argument for both signals. This feels redundant, but I'm not 100% confident there is a very good solution. This could end up being a major issue on its own, so I'm leaving it for now.
  8. Future feature: it would be really cool to allow creation of signals directly from a SymPy expression, if the user happens to have it installed.

If you have some feedback about all of this I'd be interested to hear!