NOTE: I mistakenly wrote “patch” in the title and in the text below. I meant “preset.” Sorry for confusing things.
I’m facing a challenge as I add some new params to an existing module.
If a user loads an old patch, Rack sets the values of the params saved in the patch, but leaves new params unchanged.
I would like my module to have an opportunity to set the new params to values that make sense for the old patch. For different params, the appropriate value might be:
A default value.
A value based on the values of old params in the patch.
None of the old versions of my module saved any “data” at all in the JSON. If they had, this would be easy for me to handle. Rack would call my module’s dataFromJson(), which could detect the old version and do the right thing.
Alas, no. There’s no “data” in the JSON. And so Rack never calls my module’s dataFromJson(). And so my new params remain at values that have nothing to do with the loaded patch.
My only solution at the moment is to rely on deprecated behavior: Override ModuleWidget::fromJson(), and add a “data” object to the JSON (if there isn’t one already) before calling the base class.
There’s an open issue that would help: In onBeforeFromJson() I could set a field in the module to indicate patch version 0. In onAfterFromJson(), I could detect the old style patch by checking that field.
Another possibility is for Rack to call dataFromJson() even if there’s no data (passing either nullptr or an empty JSON object). But for all I know, that might cause lots of other folks’ modules to explode.
Is there something better I can do? I would like my module to be able to supplement old patches with new smarts.
I do some awful thing to do this in my Kitchen Sink module. If you look there you can see it. Look for files where Comp::PATCH_VERSION_PARAM is used. It’s pretty bad, but it works and it doesn’t use the deprecated API. Yes, it would be handy to have a “before” callback.
btw, I think this is one of them main reasons people smarter than me do all their own json deserialization, I think it makes this kind of thing easier.
Hmmm. I may have confused things by saying “patch” when I meant “preset”.
If I understand right, your version param trick handles loading old Rack patches, but not loading old WVCO presets.
When Rack loads an old patch, it creates fresh modules, which initializes them before loading the JSON. During initialization, WVCO resets its version stamp to 0, which allows checkForFormatUpgrade() to notice that its an old patch.
My guess: When Rack loads an old WVCO preset, it loads the JSON into an already running module, and so WVCO never gets a chance to re-initialize the version stamp, and so never converts the old waveshape gain.
I can’t think of an elegant solution in code to your problem, your summary is pretty accurate. Your only way in would be ModuleWidget::fromJson which is deprecated and I wouldn’t suggest.
I have two suggestions in a different direction though:
Don’t add new parameters to the existing module, use an expander instead and put the parameters there. This way you can initialize the parameters whenever it’s attached to a „master“ module or whenever you want. You have to do this logic by yourself though.
Don’t add new parameters to the existing module, create a „mark 2“ or something of the module with additional parameters (and a data field in the JSON from the beginning). You could even implement a custom context menu option for importing legacy presets from „mark 1“.
I thought of a way to do what I need, without relying on the deprecated ModuleWidget::fromJson():
Choose an old param to use as a “sentinel” param.
Create a new param identical to the old one, and move all uses of the old param to the new one, leaving the old param unused.
In the module constructor, give the old param a “sentinel” value that was not possible in old presets.
This is Squinky’s trick, with a twist: Rather than adding a new param to use as the sentinel, co-opt an old param. That way, when an old preset is loaded, it overwrites the sentinel param with a non-sentinel value. A new preset will have the sentinel value. An old preset will not.
Start process() by checking the sentinel param. If its value is not the sentinel, we must have just loaded an old preset, so migrate the params.
I don’t like this, because contorts my module in a few ways. First, it wastes a param, and uses the param for a responsibility that really has nothing to do with parameters. Second, process() has to do stuff that is none of its business, and on every single sample it must account for an event that happens very rarely, and that would better be handled as an event.
But I dislike it less than creating a new module every time I think of new features that require migrating old presets… especially when I know exactly how to migrate old presets to take advantage of the new features.
I haven’t decided yet: Which do I dislike least? Using deprecated behavior, or contorting my module to put responsibilities where they don’t belong?
This would be only necessary once if you store the version or any other data in JSON which ensures that Module::dataFromJson will be called on loading.
Lesson learned: Always override dataFromJson() to create a “data” field. I don’t even have to put a version field in it at first, because the lack of a version field acts as a version field.
The contortions are awful, but I wouldn’t worry about the overhead of doing it in process every time. If it takes time (which it won’t) do it every 4 samples, or 16 samples… Or set a flag and set if after you do it. But, yeah, the whole contortion thing is bad, it’s complicated and a maintenance headache.
I’m applying this lesson now, and so I wonder…should this be always override dataToJson()? Does dataFromJson() create a data field? I would expect that it just reads, and so creates nothing? But I haven’t seen it in action, so I’m (as usual) prepared to be wrong.
The thing about it is that you know it was called. That’s the ticket. But the other way I think was mentioned above - just make a param that declares that version you are. That works, too. And can be much easier. Either way, you can know definitively that it’s an old patch loaded, and you can fix it up.
Ah, now I see. Not having any “data” section meant that dataFromJson() wasn’t called, and so overriding it didn’t work in the most important case.
To paraphrase, including dataToJson() in my initial release (coming Real Soon Now) means that, if I ever need it, future releases can override dataFromJson() and know that it’ll be called.