Are there any plans to fix getSizeInUnits() of the valuetree undoable actions? I’m running into serious problems because the SetPropertyAction doesn’t take into account sizes of MemoryBlocks when calculating the size it uses. The code currently is:
int getSizeInUnits() override
{
return (int) sizeof (*this); //xxx should be more accurate
}
The problem is that oldValue and newValue are of type var, which is a union that uses a pointer for memoryBlocks and thus the number of bytes used by the blocks is never taken into account.
Also looking at the undo code… why are there oldValue and newValue in SetPropertyAction? That seems rather wasteful. If I set a large memory block, this means it always uses twice the memory? In my opinion a undo system always only needs to remember the “other” state. So when performing the action it needs to store the old value and when undoing it needs to store the new value, but never both. Of course that doesn’t matter so much as long as types are small, but var can have any size.
No it’s still not necessary. Before undo the action should hold the oldValue and in case it is undone, it should hold the new value for redo. I designed an undo system for a graphics application in the past and it’s not necessary to store everything twice.
True, you can implement it that way. But I think it wasn’t designed with the idea to store a big state but rather scalar values that change. These are grouped using beginNewTransaction().
But yes, you can use it that way, and in that case some optimisations seem sensible…
I leave that to you and the juce team…
The UndoManager is based around action objects that can undo/redo themselves. It just wouldn’t be possible to apply this optimisation using that architecture, it’d require a completely different type of undo manager system.
And it’s not a huge overhead anyway - they’re var objects, so if they point to e.g. a string or large object, then the two copies will actually just reference the same shared object, meaning that the duplication wastes just one pointer.
Hi Jules. I guess I am missing something, but to me it looks like the var stuff using MemoryBlock is always making a copy of the data when assigning or using any of the MemoryBlock constructors. Otherwise MemoryBlock would need to be immutable and a reference-counted object I think. I can find no mechanism that would allow multiple vars to reference the same MemoryBlock data.
oh yeah, I was talking about strings really, not memory blocks.
But like I said, it’s not something we could feasibly optimise. If you’re designing a single-purpose undo system then you could architect it differently, but because this one interleaves a sequence of possibly heterogenous actions, each action must contain enough info for both undo + redo.
I don’t agree and have designed an undo system for a vector graphics drawing app in the past and honestly I don’t understand the argument. For undo to work the sequence order cannot change and you can never have the same action undo or redo twice. So each action can only have two states. It’s either “done” or “undone” and to switch between these states it only needs the other data.
I believe I could easily redesign the ValueTree SetPropertyAction to use only one var with the current undo architecture. If you disagree, please explain why you need var newValue once the action has been performed? If it’s just for redo, you could read it again when doing undo. It cannot have changed, because that would break undo for sure.
The newValue member only shows up in the ctor, perform() and createCoalescedAction() in the code.
If you want I can write you a SetPropertyAction with only one var member.
That would require the actions themselves to be mutable, and to swap their value with that of the target data they’re modifying. But IMHO that’s a dangerous design and I specifically wanted them to be immutable.
For example, they could be controlling something less directly than just copying a var, and repeatedly reading/writing a property could make the value drift. (Actually, this could even cause errors to creep in with a var if e.g. reading and writing it causes it to be converted from string->double and back multiple times).
This may be true for the general case, but not for ValueTree and SetPropertyAction where it’s always a swap between a var and another var and thus no conversion does ever take place. One beauty of an action/transaction undo system is that each action can use it’s own logic to perform undo and redo and if the action is known not to cause any drift (or similar issues), there is no need to keep the data twice. In fact, the swap logic could be limited to vars of type MemoryBlock because the others use only very little memory.
I guess this means I’ll fork the JUCE ValueTree/Undo system and change it myself to work much better with ValueTrees containing MemoryBlock. I have two major problems right now. The UndoManager isn’t taking into account the size of the MemoryBlocks contained in the undo system and it’s using two times the memory that would be needed. This forces me to limit undo steps to a very low value instead of making things memory-limit based. At least I think these changes won’t be large. In case anyone needs to fix the same issues, I’ll be happy to share results once they are done.
TBH I think that if you’re really serious about optimising the memory use, then a better plan would be to make those UndoableActions smarter, and to hold their before/after values inside shared, ref-counted wrapper objects, so that multiple actions would share the same copy of the memoryblocks.
I’m pretty sure you could do this without needing to touch the UndoManager itself, because there’s a callback called createCoalescedAction() which could cunningly examine its neighbouring action and abstract out the duplicate values.
I agree, but I’m scared to change MemoryBlock and/or the Variant type. These are much more low level than the ValueTree Undo actions. If I change just the actions I think I can keep the changes contained to one file and have less risk of damaging anything else.
