Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch load/save #21

Open
d3cod3 opened this issue Apr 4, 2020 · 22 comments
Open

Patch load/save #21

d3cod3 opened this issue Apr 4, 2020 · 22 comments
Labels
enhancement New feature or request

Comments

@d3cod3
Copy link
Owner

d3cod3 commented Apr 4, 2020

  • better loading/saving/autosaving model
@d3cod3 d3cod3 added the enhancement New feature or request label Apr 4, 2020
@Daandelange
Copy link
Collaborator

Currently, objects that save open, search and write/read to files individually.
I don't really understand the mechanics of setCustomVar() on objects but it looks saving related, isn't it ?

bool PatchObject::loadConfig(..., ..., string &configFile)

I suggest that all objects can save and load to/from an (preconfigured) XHML handle.
This way, ofxVP would instantiate + setup an XML handle, and pass that to object->loadConfig(xmlHandle), which in turn can call param->loadConfig(xmlHandle) and xmlHandle->doMySavingStuff().
Functional implementation fragments :
KarmaMapper/Effect/loadFromXML - KarmaMapper/Controller/loadConfig

Also, how do we handle parameter values that can be overridden by inlets ? Sometimes we want to save last inlet value, but mostly we want to save the last manually-entered value, no ?

@d3cod3
Copy link
Owner Author

d3cod3 commented Apr 7, 2020

Great idea, yes, right now, the handle is the string filepath, passed to every object, that independently load and save their chunk of code inside the xml. Having a xml handle is definitely a better choice.

And about the second question, no, right now parameter values saved into the xml are not overridden by inlets, only by GUI. That means that inlets without a GUI do not save their state in the xml. As you say, the really important data to save are the last manually-entered ones.

@Daandelange
Copy link
Collaborator

Slowly getting to saving functionality in #22 ... Any new ideas in this area ?

Do we stick with ofxXmlSettings (based on TinyXML) ?

  • It's very basic but simple; and it does the job.
  • Looping multiple xml tags feels slightly redundant, an iterator-supported library would be great.
  • Feels a bit limited (less future proof?)

Another option is ofxPugiXML based on [an old] PugiXML.

  • PugiXML is known to be way faster then TinyXML.
  • Code looks less simple, while comprehensible.

Also, regarding the karmaMapper approach (above), it would be great not to pass the full XML handle (mosaicNodes could mess up each other // security).
Possible solution : Pass an empty handle which, once filled by the mosaicNode, is appended to the final output XML handle.

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 18, 2020

I like the idea of having a faster xml library, if PugiXML is really way faster, we can take a look at it, and check if the added code difficulty is worth or not.

About the full XML handle, let's clarify how is working right now:

  1. ofxVisualProgramming have the string currentPatchFile, a reference to the XML patch file absolute path
  2. every loaded object receive the same string on creation, so they have the reference to the same file ( var string patchFile)
  3. the Mosaic XML template:
<github>https://github.com/d3cod3/mosaic</github>
<www>https://mosaic.d3cod3.org</www>
<settings>
    <output_width>1280</output_width>
    <output_height>720</output_height>
    <dsp>0</dsp>
    <audio_in_device>0</audio_in_device>
    <audio_out_device>0</audio_out_device>
    <sample_rate_in>44100</sample_rate_in>
    <sample_rate_out>44100</sample_rate_out>
    <buffer_size>1024</buffer_size>
    <input_channels>0</input_channels>
    <output_channels>0</output_channels>
    <bpm>120</bpm>
</settings>
  1. an example of patch with two objects and one connection between them:
<!-- ............ -->
</settings>
<object>
    <id>1</id>
    <name>audio device</name>
    <filepath>none</filepath>
    <position>
        <x>558.000000000</x>
        <y>538.000000000</y>
    </position>
    <outlets>
        <link>
            <type>4</type>
            <name>IN CHANNEL 1</name>
            <to>
                <id>2</id>
                <inlet>0</inlet>
            </to>
        </link>
    </outlets>
    <vars></vars>
    <inlets>
        <link>
            <type>4</type>
            <name>OUT CHANNEL 1</name>
        </link>
        <link>
            <type>4</type>
            <name>OUT CHANNEL 2</name>
        </link>
    </inlets>
</object>
<object>
    <id>2</id>
    <name>audio analyzer</name>
    <filepath>none</filepath>
    <position>
        <x>968.000000000</x>
        <y>229.000000000</y>
    </position>
    <outlets>
        <link>
            <type>2</type>
            <name>analysisData</name>
        </link>
    </outlets>
    <vars>
        <var>
            <name>INPUT_LEVEL</name>
            <value>1.000000000</value>
        </var>
        <var>
            <name>SMOOTHING</name>
            <value>0.000000000</value>
        </var>
    </vars>
    <inlets>
        <link>
            <type>4</type>
            <name>signal</name>
        </link>
        <link>
            <type>0</type>
            <name>level</name>
        </link>
        <link>
            <type>0</type>
            <name>smooth</name>
        </link>
    </inlets>
</object>

So, every object append a block on the XML, with basic info, id, name, position on the canvas, filepath ( sometimes used, sometimes not, specifically for file related objects ), a list of outlets ( with the wires ) and inlet ( fixed in most cases, variable for some objects with reconfigure inlets/outlets capabilities ), and finally a list of vars, custom and different for every object needs ( as you see in the example, audio analyzer object have two knob in his gui, which value is saved in the XML as reference for restoring the object when loading the previously saved patch )

Sin título

And every object just access his block into the XML ( via the id ), never the others.

Now, from ofxVisualProgramming, we access the entire XML, for modifying the main patch settings, for deleting a link or a group of links, deleting an object with all his current links, etc...

I don't see the problem with the full handle ( or maybe i misinterpreted your question ), but i think this mechanism is pretty similar to what you propose?

Anyway, let me know is this clear the idea of loading/saving, and is you think it can work with the new implementation.

@Daandelange
Copy link
Collaborator

Thanks for the details, and yes the mechanisms are similar.
The full/partial handles I'm talking about is more about the (optional) idea of "sandboxing" it for security.
Instead of giving a file-level access to each object, they get an XML handle to access the file data trough.

// (pseudo code)
mosaicSaveHandle = XML(file_path);
saveMosaicXML( mosaicSaveHandle );
for(object in patch->Objects){
    objectSaveHandle = new XML(); // sandbox
    object->saveToXML(objectSaveHandle); // cannot access/mess other object data
    if( objectSaveHandle.isValidXML() ) mosaicSaveHandle.append(objectSaveHandle);
}
object::saveToXML(handle){
    baseObject::saveToXML(handle); // name, id, etc.
    for(param in this->params){
        param.saveToXML(handle); // append params
    }
    handle.append(customStuff); // other ?
}

Something like this will definitely work with the current implementation, with or without sandboxing.
Maybe there will be no more need to pass de xml path to the object. (2. in your above comment)
Or has the patchfile location [sent to each object] also to do with a kind of working directory feature ?

There's nothing like continuous saving, right ? (is the XML written on any [param/other] change, or only when the [entire] patch is saved [autosave or manual] ?)

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 20, 2020

Right now it is autosaving, the XML is automatically written on any param/other change, but it is a good idea to have both options ( autosave and manual ) so the user can choose his style

About the sandboxing, do it as you see it better, just a silly question, the append is perfect for creating a new object, but what when we edit a previously created object?

We have here the basic logic CREATE/LOAD/EDIT/REMOVE

the sandboxing is just for the CREATE part, right?

And about the patchfile location, yes, is needed for objects with loading file capabilities for extracting the working patch data folder, this var in patchObject: string patchFolderPath . The mechanism is that when you load some file in an object that have this option ( video player, soundfile player, lua script, etc...), the file is copied inside the data folder at patchfile level and the object filepath in automatically updated to that, so a patch in independent, you can open it in another computer, at start it will refresh all the file absolute paths for the new system path and everything will load without problem.

@Daandelange
Copy link
Collaborator

Daandelange commented Jul 20, 2020

Ok, just updated ofxPugiXML : https://github.com/Daandelange/ofxPugiXML
I'll give it a try.

But first, I didn't realize that Mosaic uses autosaving. Then my method won't work; I'll re-consider my proposal.

Let's keep the sandboxing idea for later. (it would be where possible)
And I don't understand everything about the file path and/or working dir, I'll dig into the code to understand it better.

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 21, 2020

The working dir is for patches that loads other files in some of his objects, having a data/ folder ( i called it like this to maintain some relation with OF ) is to porting the patch from one computer to another, basically:

i made a patch in my laptop with linux, i want to share it with you, the patch named testingPatch is located in a folder with the same name

testingPatch
> data
- testingPatch.xml

and inside the data folder are stored all the files used inside the patch (video files, script files, sound files, etc...)

so i zip the testingPatch folder --> testingPatch.zip

send the file to you

all the needed files are there in the data folder, but the paths ( absolute ) in testingPatch.xml are still the ones working on my laptop, but no problem, at opening the patch, thanks to every node knowing filepath and working dir ( locally ), the paths will be refreshed before loading the nodes into the canvas

so you'll have my patch working in your machine without problems

@Daandelange
Copy link
Collaborator

Ok, that I understand that part.
But how is (auto)saving handled precisely ?

As I understand it, when customVars are edited, autosave is triggered. (Or manual save, not an issue)
PatchObject::setCustomVar(float value, string name){ customVars[name] = value; saveConfig(false); }
There seems to be no way of editing the XML file on disk (in place), it always happens on the DOM structure, which once edited, is integrally written to a file or a stream.
To me, it feels a bit "heavy" to write the whole XML file again to auto-save 1 parameter in a node. But the current Mosaic version demonstrates that this technique works. Personally, I'll definitely stick with manual saving; eventually automatic timed incremental saving. But it's a nice feature to have, to be able to choose.
Another approach to autosaving is keeping the DOM XML tree always up-to-date with params, but write it out only from time to time (interval?).
What do you think ?

Now about the file paths and working directory stuff, I see that each object stores an instance of the save file location. std::string PatchObject::patchFile
Shouldn't this be in a Mosaic::Patch level ? (maybe a singleton for getting MosaicPatch information?)
Something like Mosaic::GetPatchInfo().PatchFile(), Mosaic::GetPatchInfo().DataFolder(), Mosaic::GetPatchInfo().LoadedXmlDOMTree(), etc.

Anyways, playing around with PugiXML, it definitely offers more options to traverse and edit xml trees.
The API is nice and easy to understand.
Btw, they say TinyXML2 is 4x to 10x slower @ parsing documents. On the other side, the memory footprint is a little smaller for TinyXML. No comparative tests for the DOM writeout, but as it's cached, that should be limited by the disk speed and std write functions.

Note: TinyXML and PugiXML are both DOM parsers, which means that the loadable files are limited by the available memory (RAM). (not an issue for us, except if we target very-small-memory-devices ?)
This also why they are amongst the fastest parsers.

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 21, 2020

Well, the write is not over the entire xml file, if you check the saveConfig(bool newConnection) function, you'll see that opens the file, traverse it and only access and rewrite the object xml node ( <object> ... </object> ) specified by the ID, so is just a partial rewrite of the file.

Optimized, no, is not the best solution, was the fastest one at the moment, i was constantly running in the first two years of Mosaic development, due to the strict relation with the use of the software at the university, so there's a lot of stuff "fixed" to just work, and that need a better logic.

I like your idea of update the xml from time to time, but i would like to maintain all the possibilities: manual saving, automatic saving every N minutes, and constant auto-saving, having the auto-saving yet implemented, it will not be a problem, and we can adjust the logic to obtain a better code.

And the limit over the RAM, not a problem, Mosaic patches are really small, the heaviest object in Mosaic ( sequencer, 5 inlets, 21 outlets, 64x5 = 320 custom vars ) is 34kb in the .xml, and a patch with 60 objects loaded is usually around 60 kb, so we can talk, as average ( minus some specific objects ) 1 kb x object

So it's not an issue, considering that this kind of software is not designed to run on a very-small-memory-device (it would be nice to have it compiling for arm64 and running on a raspberry pi4)

@Daandelange
Copy link
Collaborator

Let's go for several save modes then, starting with auto, the most complicated indeed.

Btw, no, it's a detail but it's a total file rewrite. You don't parse all the XML tree, but you traverse it as needed, then you edit it where needed (modifications go to the DOM buffer), then write the whole DOM to a file.
There seems to be no other way with DOM-based parsers (the fastest ones). Others, I haven't verified.

if(XML.loadFile(patchFile)){ // puts file buffer in DOM buffer
    // [...] do stuff with DOM buffer (traverse, modify)
    saved = XML.saveFile(); // writes DOM to a file
}

One more question, why is <filepath>none</filepath> in <object> rather than in <settings> ?

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 21, 2020

Oh, so i always misunderstood the DOM buffer, well, better later than never, thanks for the clarification!

The <filepath>none</filepath> is not the absolute path of the patch file, is the absolute path of the specific file loaded in objects with file loading capabilities ( most objects will have this value always with none ), so it's obviously an <object></object> var

@Daandelange
Copy link
Collaborator

Ok, I see, but then filepath is a custom object var, not a default object var, no ? If so, there could be a fileParameter which specific objects can use.

Also, I feel we're gonna end up with a new xml syntax, making older patches incompatible. (almost inevitable)
For example, I think it's better to wrap <object> nodes in <objects>, and also make better use of attributes like <object id="1" name="SimpleRandom1" type="SimpleRandom">. This will make XML traversing faster (ex: no need to move into <object> to get the id).
Also, with the new parameters implementation, the parameters of an object will define the inlets and outlets, holding their name, value, connected links... So no need to store <outlets> and <inlets>; and <vars> will become <parameters>, and they'll save themselves.
Does that sound right ?

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 22, 2020

Yes, i was preoccupied by that, but i too think the incompatibility with previous versions is inevitable.

For the filepath, yes, is a custom object var, used only by a bunch of objects, so it can be a fileParameter, or a simpler stringParameter.

For all the rest, all good ideas, and good restructuring of the xml, it sounds right!

@Daandelange
Copy link
Collaborator

I was wondering if there's a way to write only changed nodes in a file, TinyXML still seems to be the best choice.
RapidXML's DOM mechanism in fact serves pointers-to-data instead of data-copies, compared to TinyXML and PugiXML. (they call this an in-situ parser). That sounds good, but when it comes to modifications, it seems to be a heavy process offsetting all data for a simple direct-file-change : even RapidXML rewrites the whole buffer to stay rapid.
So I guess it's a foolish wish to write changes real-time.
And RapidXML seems harder to write.

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 23, 2020

Thanks for searching for better options, it seems that the best choice is to stay on TinyXML and ofxXmlSettings

Adding some features as sandboxing and maybe some better logic, i'm sure we'll have a more than satisfactory result, until now there were never issues with the xml load/save stuff.

@Daandelange
Copy link
Collaborator

Whoops, sorry, I meant to say PugiXML is the best choice : a little faster and more future proof than ofxXmlSettings/TinyXML.

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 23, 2020

Ok then, we'll switch to PugiXML then, maybe we can write some methods to have it imitating ofxXmlSettings functions, so the port will be super easy?

@Daandelange
Copy link
Collaborator

It will be super easy, it works the same way with better attribute support and more traversing options.
The only 'weird' API change is that you call xmlNode.getText().value() and xmlNode.getText().set("new").

@d3cod3
Copy link
Owner Author

d3cod3 commented Jul 27, 2020

I've pulled some basic api for ofxPugiXML

Daandelange/ofxPugiXML#1

to mimic ofxXmlSettings and simplify the future Mosaic port

@d3cod3
Copy link
Owner Author

d3cod3 commented Aug 14, 2020

Just discovered that pugixml 1.9 is included in OF0.11, and ofXml class is based on that, maybe we could extend the class and use that instead of the ofxAddon?

@Daandelange
Copy link
Collaborator

Daandelange commented Aug 16, 2020

Yes, or make the ofxAddon use the embedded library, depending on how much ofXml restricts pugiXML's capabilities.
Weird that we didn't run into any duplicate symbols linker errors.
Weird that OF provides 2 different XML APIs. (since 2016 already, some interesting notes here )
https://github.com/openframeworks/openFrameworks/commit/a7006b16f10d19d81054cb6c6da46bd4a73f9cc0
So ofXml uses pugiXML while ofxXmlSettings uses tinyXML.
Note: Currently, ofXml uses v1.9 [04-2018] while ofxPugiXML uses v1.10 [09-2019], should not make a big difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants