Add wt import from oscdata to wavetable script state#8194
Add wt import from oscdata to wavetable script state#8194nuoun wants to merge 15 commits intosurge-synthesizer:mainfrom
Conversation
|
So the code looks fine but let me add the workflow question I added on discord last night. Let's say you take a wavetable and do a simple transform like amplitude -> amplitude * 0.5. Whatever. If you run the wtscript to generate 4 times you will end up getting like .00625 the amplitude since the 'prior' isnt' the unmodified table its the currently loaded table. Am i missing something? Seems we need to keep a cache of 'original table' around somewhere for this feature? |
|
Yes it is destructive and unlike processing in an actual audio/wt editor there is no undo state to go back to either, I considered the use scenario where a wavetable in one of the other oscillators gets used as the source here which would persist but that is perhaps not very intuitive. With a cache the question becomes what is the most intuitive way to keep a cache of the original WT around and how to manage that, would it need to be saved with the patch? |
|
I think its pretty important that seemingly idempotent scripts produce consistent results, yeah? But yes this would stream twice. Ugh. Answer not obvious. Let's ponder a bit.... We could persist the original with the wtscript. but fragile and stateful. |
|
So I like the possibilities this opens up combined with the simplicity of this approach but after giving it some more thought I think what is objectively bad here is that we're saving the script in .wtscript format or as metadata in .wav/.wt but this does not have any of the oscillator input data so how it's used here is not reproducible or as you put it with a word I had to look up it's not idempotent. So back to the drawing table and I think a different approach altogether is instead of using Surge's oscillators as sources it could use two different import slots that can load a wavetable. Those slots could be set from the file/hamburger menu and get saved with the .wtscript, patch or metadata for .wt/,wav (the reason for it being two slots is being able to copy from two tables or do things like convolutions). With that approach the issue becomes how to save the imported wavetable data in all 3 different formats: Surge patches, .wav/.wt metadata and the .wtscript XML, a lot more complicated than I had hoped but also a lot less hacky. |
|
Oh sorry idempotent is a word from language nerd and services nerd world I should have been clearer |
|
I read the code and comments now. Agree using the wavetable oscillator contents directly is not a good idea. My thinking is the script editor could have a data cache with a persistent name, which is empty by default but can load data from a wavetable. The basic level is as we discussed, user loads a table, the frame count and samplerate gets set to that. |
|
That seems to be where you're headed also. That data could be read-only, which would make many things simpler. Indeed there's no reason we need to limit to just one of these, but whether there's one or more I think a fixed set with good names is good UX for the script writer. A possible next level from there is to have two representations of each loaded table. One is the constant read-only data from the wavetable itself, which the script can't interact with directly, the other is the Lua table that the user sees, and is computed with 2d interpolation from the wavetable size to the number of frames and samples the script UI is currently set to. That would be very convenient, but a bit trickier of course. |
63011db to
5f8a2eb
Compare
|
IMO those snapshots don't need to be saved to WAV or WT formats, I don't see the reason to. You're importing WAV or WT to the snapshot, not the other way around - I'm not sure I see what problem would storing snapshots to WAV/WT solve. So it's fine if we don't have that. It is, on the other hand, obligatory that snapshots are saved along with .wtscript since they are instrumental in having that .wtscript fully portable. |
|
I added two unittests, maybe @baconpaul and @Andreya-Autumn have something to add otherwise I think this is ready for review (unless we want to rename this to something other than snapshots). |
|
Oh i'm sorry I'm just reading this now. I'm not sure why you did a few things First, the WaveTableSnapshot structure seems basically identical to the Wavetable structure. Why is there a new structure? Second, storing the wavetable as base64 encoded floats in the patch is really really expensive. Could we not have done the same thing we did with convolution and add more binary storage at the end for snapshots? Finally I'm not sure why the snapshot is in the daw extra state at all. Wouldn't you need it to make the patch work if you share the patch outside of a daw context? The WTSE will do the wrong thing without the snapshot loaded. So I think we need to think a bit more about serialization and workflow here. |
|
Yeah I was just reading this too and I agree storing in DES is a bit strange. The patch would still play correctly once the wavetable is generated. But everything else about the script saves with the patch, so users may expect to be able to open the script, make a small change, and re-generate. As Paul correctly points out that operation is non-portable as it stands now. And I agree doing what Surgo did for the convolution seems like the right call. I also wonder if we can do something about frame count mismatches, but that's a problem we could deal with later. Other than those things I think this is much improved over the original proposal. |
|
Thanks for the comments, responding to them one by one:
Mostly because it's structure is an array for the snapshot slots, has an extra bool plus a lot of things in the Wavetable struct that are not needed for this, seemed different enough at first but I agree there's no reason not to use the Wavetable struct in another, snapshots dedicated struct.
It's currently not storing in the patch at all (patches save the scripts, but only use wavetable data set in oscdata directly) , the base64 is just for saving and loading to the .wtscript format but I agree base64 does seem overkill since these are just floats, the floats bytecode still needs to be encoded for XML so I'll have a look at how Convolution does it.
That seemed like the logical place since we store a lot of Lua editors stuff there already, I'm not sure what a better home would be for it to be honest, it's not DSP so probably not OscillatorStorage. Suggestions welcome.
The initial idea is that this is where .wtscript comes in, we can dedicate that format and that format alone to saving snapshots without adding a lot to the size of patches only for this feature, seems worth it imo, since otherwise it's also a footgun, a user will have no idea why a patch is ~16MB larger because deeply buried in a feature of a feature it's storing data that only a few users might find useful. So, again suggestions welcome.
Having no size constrains for frames and samples (other than it needing to be valid wavetables) means the scripts can do things like copy parts of to a new wavetable with different frame counts, or up- or downsampling to a new table. Limiting this would probably also limit functionality I think. |
|
I see your point about patch size. Here's the tradeoff we're weighing though: User A somehow notices that a patch their friend sent them is way bigger than it should be. "That's weird..." they think. I think the weirdness experienced by user B is way more important than that experienced by user A. It is already a fact that some Surge patches are way bigger than others based on what features are used. User probably don't even notice or care about that 99.99% of the time. |
|
And yeah you may be right about the frame size and sample count mismatches. It's not a blocking concern here anyway. I'm more thinking maybe we can eventually add convenience functions to help interpolate or wrap etc etc. But that's for later! |
|
I mean the ultimate solution is of course referencing wtscript files rather than embedding them... |
|
If @mkruselj agrees I will happily make it that the snapshot data saves in all three* locations. But right now I'm not sure if we all agree on that. (*: The third location being metadata in .wt/,wav, a novel concept of having wav data as metadata there but I think it will work.) And yes, worth pointing out maybe that in the above scenario, with the current implementation, the patch would work fine, which is probably all that 99% of users need, but the included script would error when trying to regenerate the WT at which point user B could ask for the .wtscript (of course the point stands, user C who downloads the patch and has no way of contacting user A will still be out of luck),
Yes maybe, wonder what that would look like in practice, something like generating a hash of the script, and load that if it can be found in either the factory or user folders? Or make it so that the .wtscripts needs to be in the same folder as the patch? Could make sharing patches that include the wavetable scripts a bit harder though. |
|
Could simply scan the WT folder for all wtscript that match by name first then hash second, I suppose? I would not want to mix wtscript and FXP in the same folder. "Simply", says he... |
|
I think the solution is "just store the stuff with the patch". It's fine. No one cares about 5-10mb of storage here and there in 2026. Except maybe the rPi/embedded folks, but they are already succesfully dealing with it anyway. The .wav metadata thing is interesting! But a bit involved, potentially bug-prone, and not really a solution to any problem we currently have I think? So I contend let's not do that for now. In the unlikely event someone eventually submits 300 fantastic patches which all rely on a 15mb wtscript, we put those .wtscripts in a separate download location like we link the Adventure Kid/EMU HQ/etc wavetables, and flatten the patches to use the normal wavetable outputs. Then in XT2, we do a bigger restructure to allow wavetables, scripts and IRs all to be referenced instead of stored. That makes sense to me anyway. |
|
I think it is very uncool to have a soundbank of 100 patches that could potentially end up over a gigabyte in size. It's a no go from my side. I'm not even talking about anyone submitting such a soundbank to us. It could be somebody distributing it on their own. I still think that is unacceptable. .wtscripts should be referenced and snapshots should not be stored in a patch, that's my 2c. |
|
Let's zoom out a bit. Only in the worst cases (2-3 script oscillators, each using more than one slot, each slot loading the largest size wavetables we load at all) would the scripts add more weight to the .fxp than the convolution does with my piano impulse loaded. The likelihood someone makes, for distribution, 100 patches with that Piano IR, or 100 of those very heaviest wtscript patches, are both non-zero but probably really low (I'm sure we agree). But if one of these is unacceptable the other is also. The same solution applies to both (and wavetables in general), and I agree it's a good one. But do we need make that much bigger infrastructural change before XT2, or don't we? A positive answer seems likely to delay XT1.4 another couple months. And how long after that before it's time to start working on XT2 anyway? I thus judge store with the patch as "good enough for now". |
|
I haven’t been following but - Why would we store it anywhere other than the patch? The patch is self contained and fully renderable and editable. If we don’t meet that constraint don’t merge the feature |
I agree. The others' concern is "it can make patches really big". A valid concern indeed but, yeah see above. |
|
yeah i get that. that's a good reason to not merge the feature perhaps. i don't think it is but if someone wants to make that case i'm ok but if we add items-by-reference to patch we need
thats not un-doable, but item 2 makes it way harder than short circuit and its a lot of code to write. the thing which isn't ok is to have patch have reference to external data without doing and testing all that work. |
|
I don’t see why adding another wt or two to the wt section of the fxp isn’t just the answer tho? |
|
Yeah I agree. I don't think you need to drop it! |
|
I looked into binary storage a little and if I nerf it down to two slots instead of the current three I think it will at most (so two max size WTs) be a ~8MB patch size increase (and worst case ~6-7MB with zstd, it tends to compress poorly). For the average use scenario it will be much less of course so I also think it's fine but only if there are no objections, I rather do this by consensus or not at all so please let me know @mkruselj. |
|
those slots will only be used if (1) you use wtzscript and (2) you use it to transform an extant wavetable right? i see absolutely no problem with that. |
Yes on both, so I think this is the plan if there are no objections:
|
|
Sure I guess I can get behind that. |
|
I get it all except for the 2 slots part. Why not allow n slots? |
|
To make things like carrier / modulator modulation possible, or spectral morphing from WT 1 to WT 2, or as simple as being able to procedurally copy two tables to a new table. Without having at least two it's kind of limited. |
|
I think the question was "why not 3". |
|
I think that last proposal sounds good. And would also be fine with keeping the three slots but if it alleviates some of the file size concern I am not attached. |
|
Oh, misread that, yes what Andreya said. Two tables covers most use cases anyway and if more tables really are needed the feature can at least be used to copy new tables one at a time. |
|
To elaborate slightly, there is imo kind of a poor design choice here where a patch just keeps saving the snapshot data no matter what as long as there is data in the snapshots struct (which UI is buried rather deeply). I don't have a good alternative for it since I'm pretty sure we don't want a pop-up on patch save that asks confirmation on also saving snapshots so I feel a little better by limiting the amount of slots available but suggestions welcome. |
|
Ok I think this is in pretty good shape now, per Discord discussion the structure that was favored was to add the WT class struct to OscillatorStorage so each WTSE instance can use it's own snapshots. I first tried Other changes made since this post:
|
|
One UI suggestion for the save dialog: Put checkboxes horizontally next to each other then OK/Cancel buttons below them. Should look a lot nicer. Also maybe fully qualify "wavetable script" instead of WTS, for clarity. Other than that, this looks great to me! This PR possibly has the longest discussion chain out of all PRs weve ever had! |


Draft - do not merge yet.
So this adds the option to import wavetable data from each oscillator to the WTS Lua state table with an opt in so existing tables can be processed, edited, used with the PFFFT wrapper, etc.
If
wt.importwt = trueis true in init() it populates wt.osc1, wt.osc2, etc in the state table passed to generate(). The table layout is wt.osc#[frame index][sample index].Thoughts welcome!