FR: ValueTree::swapChild(int index1, int index2, UndoManager *undoManager)

We already have a moveChild() function. However, if I only want to swap two children positions, I currently need to call moveChild() twice (it also generates two callbacks). However my main problem isn’t the performance, but when you call the moveChild for the first time, the children are sorted in a temporary wrong way, until you call again moveChild().

Since the children are stored in a ReferenceCountedArray which has a ‘swap’ function already, I think it should be easy enough to add this QoL minirequest. You would need to add another callback to the ValueTree::Listener, but I suspect this wouldn’t be a burden now that they aren’t pure virtual anymore.

+1, I’d really like this feature.

FYI here is what I use to swap ValueTrees:


void swapValueTreeChildren(juce::ValueTree vt, int a, int b, juce::UndoManager* um = nullptr)
{
    if (a > b)
        std::swap(a, b);

    if (b >= vt.getNumChildren() || a == b || a < 0) return;

    vt.moveChild(a, b, um);

    if (b - 1 != a)
        vt.moveChild(b - 1, a, um);
}

It’s not very pretty, but it does work (and you the childMoved listeners will pick it up).

1 Like

Thanks for the suggestion! I do something pretty similar:

      jassert(isPositiveAndBelow(index1, listTree.getNumChildren()));
      jassert(isPositiveAndBelow(index2, listTree.getNumChildren()));
      if (index1 != index2)
      {
         const int minorIndex = (index1 < index2)? index1 : index2;
         const int majorIndex = (index1 < index2)? index2 : index1;
         listTree.moveChild(majorIndex, minorIndex, undoManager);
         listTree.moveChild(minorIndex + 1, majorIndex, undoManager);
      }

Unfortunately my biggest issue is that after the first moveChild, all items are temporary in wrong positions. If you think into single-thread, this wouldn’t matter, but while using multiple threads is a bit annoying.

ValueTree isn’t thread safe so you shouldn’t be accessing it without locking (which would prevent access whilst it’s in this temporary state).

My advise would be to really not use a ValueTree from separate threads. Take a copy, do some work then hand it back if you need to. Otherwise, only access it from the message thread.

I am not using it for separate threads. I’m using it only in the message thread (btw, for this reason, I asked in the past it may be optimized using SingleThreadedReferenceCountedObject). However its listeners send lock-free messages, using fifos and/or atomics, which are executed in other threads.

I guess it depends on the type of listener updates you’re sending but two thoughts sprint to mind:

  1. I’m not sure why having two moves would be any different to having a swap. Having as few change operations as possible makes the API much simpler and easier to compose. I’d always conceptually think of a swap as two move operations anyway
  2. If your listeners send lock-free messages, wouldn’t the ValueTree having an atomic reference count be beneficial here? I think you can then have pre-allocated messaged (required for a lock-free queue) and simply copy in your moved/swapped tree to the fifo which will be safe as it’s just an atomic-inc?

I agree with you with a simpler API is better than a crowded one. But a consistent API is still better! The most of the modern containers have a swap function. I need to coalesce the two “moves” in a single swap, and the only way I can do it is using an ugly combination of a boolean flag and AsyncUpdater::handleUpdateNowIfNeeded() to keep the sync between threads.

Your second thought confuses me, however. Do you emplace in your FIFO ValueTree objects? Anyway, if you do this, they must be deleted in the message thread, so having a atomic reference counter, wouldn’t have too much sense. But I’m not an expert on this! In fact I’ve learn about this reading you and watching you in videos :slight_smile:

What do you mean by modern containers? Like std::swap?
Isn’t this really for efficiently moving a whole container though? And as far as I’m aware it’s largely redundant now we have move semantics. Anyway, that’s a bit of a side issue…

I guess what I’m getting at is why is a swap significantly different to two moves in your case? In pretty much every case I can think of (that we have at least) two moves are basically the same thing as a swap and it’s usually easier to reason about as moves rather than a swap as there’s less to keep track of.
But again, this is just my experience, there may well be some well-defined use cases, I’m intrigued to find these out more than anything.


No… I don’t actually have any code that moves value trees between threads like this. In almost all our code ValueTrees are only ever accessed by a single thread. The only time we have multiple threads is when they’re constructed on a background thread like when creating a preset/file library or loading an Edit. When they’re done, they’re just handed off to the message thread.

I was really getting at the fact that the atomic ref counts means you can pass them between threads.
But again, it sounds like I’m missing the use case here as I don’t have any places where VT changes are propagated between threads. The only data that’s ever shared between threads is a single property via a CachedValue.

If we’re talking efficiency, then it’s worth pointing out that the method for swapping that Juan and I have pointed out involving moveChild() is really a shuffle, meaning linear complexity. std::swap would make it 0(1).

However, I think a more important consideration is that the function for swapping that Juan and I have outlined is not that easy to deduce. The +1 / -1 on the second move is tricky, as is the fact that you have to perform the two moves in the right order. There are also a lot of edge cases when indices are out of bounds. I remember it took me at least half an hour to figure this all out, and that’s a cost that anyone else who wants to swap ValueTrees is going to have to pay. But adding this feature under the hood would be trivial, since the juce::Array in the SharedObject already has a swap function.

Maybe I’m missing something, but when I encountered this problem I was stuck one step before any threading stuff: just the fact that the first move triggers all listeners is the problem. With a potential ValueTree::swap there should only be one call to listeners if the indices are not the same. Currently e.g. the code from @LiamG doesn’t even guarantee two listener calls.

This is true, but at least it triggers the valueTreeChildOrderChanged() function that is called, not the removed and added.

I’m going to show you it with an example. If you have a clever way to solve this problem, I’m opened to any suggestion. I currently have 6 banks (B) with 8 parameter (P) per bank. The interface shows only one B page at a single time and its 8 P, so you may swap P with a right-click menu. In order to simplify the problem, suppose I have only 2 B with 4 P each.

Each P is similar to an audio effect or a FM operator: the order matters. And each B applies some common properties to the P it “owns”. Actually, the B doesn’t own any P, but they are children of two different ValueTree. So the first four P belong to the first B.

The banks:
{B1, B2}

The params:
{P1, P2, P3, P4, P5, P6, P7, P8}

  1. With a single ‘swap’
    Ideally, if I want to swap P2 and P5, the final result would be this:
    {P1, P5, P3, P4, P2, P6, P7, P8}
    And I would notify to my fifo to audio with something similar to
    fifo.sendParamSwap(1, 4); // Indexes
    Now B1 would apply its common propertys to P1, P5, P3, P4.

  2. With two ‘moves’
    As we discussed, the swap can be done with two moves.
    Move 1: {P1,P5, P2, P3, P4, P6, P7, P8} → fifo.sendParamMove(4, 1);
    Move 2: {P1, P5, P3, P4, P2, P6, P7, P8} → fifo.sendParamMove(2, 4);
    It needs to queue two fifo messages. They are triggered by the ValueTree::Listener functions, which don’t know if I’m performing an action, or undoing, or redoing. So, I cannot reserve space in the fifo for two messages, I need to send them individually. I’m reading that fifo at the beginning of the processBlock(). Since audio may execute in any moment, it’s possible (and it happens!) that only process the first queued move. So the audio thread would swap the elements to Move1 position, and it will process the audio with that state (which is undesired) during one buffer. Then Move2 for the next buffer, and due to the abrupt change, it will create a cranky audio artifact. Please notice immediate changes are desired, but only once!

I hope you may understand my problem and why I miss the swap function in the ValueTree. I’m currently solving this using a weird and complex combination of flags and AsyncUpdater. But it’s far from ideal. And I can do this because I’m only swapping, not moving. Imagine how this would be if I where combining both types of reorderings.

I guess I’ve never come across that because we don’t directly control our audio processing in terms of events from the ValueTree.

Taking your example, I would do something like this:

  1. On the message thread, perform the two moves to the VT children
  2. In the first listener callback, trigger an async update (which will eventually result in a “parameters changed type of message”)
  3. When the second move happens, this async update gets triggered again. As this is on the message thread this is effectively a no-op
  4. When the handleAsyncUpdate callback actually happens, we know the parameter order has changed (it could be due to multiple moves, an undo etc. it doesn’t matter, we just know that the current state is what needs to be reflected in the audio)
  5. This would then get synthesised in to a set of parameters or some kind of processing graph that then gets atomically passed to the audio thread
  6. The audio thread then picks this up and just starts using it. It doesn’t need to know anything about what has changed really, just that it has a new graph it needs to play

It’s worth pointing out this is exactly what happens in Tracktion Engine. The user can do loads of things to the model at any time, even script multiple things. When a change occurs, a new playback graph is created which can introspect the old one and pull out any shared state like filter states, delay buffers, resampling buffers etc. so it’s relatively lightweight to create a new graph and pass it to the audio thread.

That sounds like a bit of a deviation from what you’ve got but its nice and clean. I guess in your case I might just do something similar but rather than have a sendParamMove or sendParamSwap message, have one that is just newParamOrder and let the audio thread figure it out? Then you’re only ever sending a single message no matter how many moves, swaps etc. are done (as it’s async).

1 Like

Thanks Dave for your reply! In fact I’m using a mixed system. I use the approach you described for objects which don’t rely on others like a MidiMessageSequence. But I’m micromanaging others, like, ie, a new value for a level sending it as an event to the audio thread - because it’s just a primitive.

In this case I was micromanaging this one, because, it’s tied to a variable number of sequences. I mean, think in a sequencer like this one I just find in Traktion Engine. If that sequencer would support patterns and I would wish to insert, delete or move a lane, that would mean I’d need to create an object for each pattern again. Wouldn’t it be a bit of overkill? Think pattern could have more information with a more complex sequencer.

That’s the reason I was using the mixed system.

However, I’m thinking more in your approach. Are you flagging all objects which content maybe changed and then, you rebuild only them within a single message? If you do that, it’s quite interesting. But, it triggers me a red flag. How would you manage it when you have few old tracks, {T1, T2, T3} and they are replaced by {T5, T2, T1, T6} Obviously you may have in the audio thread tracks some data you want to keep (thinking in T1 and T2). Do you manually search using loops to see which of these were the old tracks (and replace only the parts of them that actually changed in the message thread)? Or is there a clever way? Please tell me more :slight_smile:

We generally hash objects which can be moved between playback graphs so we can check quickly if they’ve changed (the has of the object will change).

Then each “container Node” also has an ID so when swapping a graph it can quickly look up and do the following:

  • “Hey, am I replacing an existing node?”
  • "Ok cool, I’ve found a Node to replace, does it contain the data I need (check the hash)
  • If yes, take a reference to it
  • If no, create my own data structure and initialise it which I might pass on to future graphs

Generally, I wouldn’t worry too much about performance to start with. Looping through some nodes or structures will be very quick if done on the message thread. Its only worth optimising with look-up tables etc. when you have thousands of nodes/objects to look through.

2 Likes

Thanks for the help. I think I can figure the remaining myself. I will modify my current project and I will apply this new concept!