Combining Xml Trees

[greebo]

I think this can be done rather easy with this function:

 

xmlNodePtr xmlNewChild (xmlNodePtr parent, xmlNsPtr ns, const xmlChar * name, const xmlChar * content)

 

although I haven't tested it yet.

 

Considering this works, how should we encapsulate this XML tree? At the moment all the game description XML is accesssed through the CGameDescription class, but this would be the wrong place for the ui.xml definitions, wouldn't it?

 

Should we create a new class (CRadiant, CRadiantRegistry, CRadiantDescription, whatever) and pack all of the existing info (GameDescription and EntityInspector definitions) into this new class? What do you suggest, Orbweaver?

[OrbWeaver]

At the moment the XML is a bit of a hybrid - the CGameDescription is used by legacy code but an xml::Document is retained for allowing XPath lookups.

 

An entirely new class is probably not needed - if the GlobalRadiant module had a single method getXMLRegistry() to return the xml::Document& corresponding to both ui.xml and doom3.game appended under a common root node (so you would have "/ui/blah" and "/game/blah" in the same Document), then modules could simply use the relevant xmlutil methods on this document to lookup XPaths, examine children etc. Eventually CGameDescription could be removed altogether, with the existing keyvalues looked up as additional XPaths.

 

Obviously the xmlutil library will need new methods to do things like append a child node, change node content etc.

[greebo]

Ok, I will see what I can accomplish, but my time will be more limited the next week than it was in the last, so maybe this will take a bit.

[OrbWeaver]

No problem. It will be quite a large task because there is a lot of code that uses CGameDescription in some way.

[greebo]

As a first step I will try to merge two XMLNodes into one. Once I figured this out, I will write the getXmlRegistry() function (and perhaps a specialist function to directly access values) and implement it with the showAllLightRadii.

 

As soon as this works all the legacy CGameDescription values can be ported over to using the new one, but we can take our time with this one.

[OrbWeaver]

As a first step I will try to merge two XMLNodes into one. Once I figured this out, I will write the getXmlRegistry() function (and perhaps a specialist function to directly access values) and implement it with the showAllLightRadii.

 

You probably don't need to "merge" two nodes as much as append both nodes to a common parent node (maybe "/darkradiant" if XML requires a single named toplevel node).

 

As soon as this works all the legacy CGameDescription values can be ported over to using the new one, but we can take our time with this one.

 

Yep, there is no urgency there.

 

Once this is done it should be a huge step forward because we can put EVERYTHING in the XML registry - colours, UI state, window positions, basically anything that is either non-persistent, non-configurable or uses the Radiant preference system (which seems to be an ugly reimplementation of std::map) could use the new system.

[Crispy]

(maybe "/darkradiant" if XML requires a single named toplevel node).

Yeah, XML always requires a single named toplevel node (unless libxml2 allows you to bend that rule, but I doubt that it does - it's better practice to follow the rules anyway).

[greebo]

Good news: I could add three child nodes from three different XML files to the node. One of these was the node from doom3.game, and the other two contained tags, so I ended up with two tags under . The result looks like this:

 

<darkradiant>
<game type="doom3" ... patchtypes="doom3 def2doom3">
<filesystem>
	<shaders>...</shaders>
</filesystem>
<defaults></defaults>
<entityInspector></entityInspector>
</game>
<ui>  
 <toolbar name="standard">
<separator/>
	<toolbutton name="open" action="OpenMap" tooltip="Open a map file" icon="file_open.bmp"/>
	<toolbutton name="save" action="SaveMap" tooltip="Save the current map" icon="file_save.bmp"/>
<separator/>	
 </toolbar>
</ui>
<ui>  
 <toolbar name="test">
<separator/>
	<toolbutton name="test" action="bla" tooltip="Open a map file" icon="file_open.bmp"/>
 </toolbar>
</ui>
</darkradiant>

This whole xml::document is stored in memory and can be assembled during DarkRadiant startup.

 

I could retrieve the toolbar named "test" via the following XPath: ui/toolbar[@name=test], so the concept works. Basically we can throw just any XML file into this global document and use it as registry - as long as the container structure is the same, it doesn't matter which XML file the information originally came from.

 

I would suggest writing functions that allow random XML files to be added with a single function call during program start, so that this unified XML structure is created.

 

The second part is writing a small API additional to findXPath to make value lookup and manipulation easy (the findXPath method is fine for lookups but its result still has to be interpreted). I would prefer to have two functions like getXmlRegistryValue("ui/state","show_light_radii") and setXmlRegistryValue("ui/state","show_light_radii","1"). All the safety checks could be delegated to these two functions, which would be much more convenient for the handling of a global variable.

 

For more complex queries like the ones in the EntityInspector the findXPath() can still be used.

 

Nothing of my suggestions is implemented yet, so what's your opinion on this?

[OrbWeaver]

I would suggest writing functions that allow random XML files to be added with a single function call during program start, so that this unified XML structure is created.

 

That's an interesting idea. I was thinking mainly along the lines of dividing things into /game and /user (probably a better name than /ui), but leaving the door open for additional config files sounds like a good design.

 

I'm not so keen on having two UI nodes though - is it possible to merge the nodes by adding all of the children of the new one onto the existing one? This way you could use unique XPaths rather than having to search by attribute.

 

The second part is writing a small API additional to findXPath to make value lookup and manipulation easy (the findXPath method is fine for lookups but its result still has to be interpreted). I would prefer to have two functions like getXmlRegistryValue("ui/state","show_light_radii") and setXmlRegistryValue("ui/state","show_light_radii","1"). All the safety checks could be delegated to these two functions, which would be much more convenient for the handling of a global variable.

 

I agree that separate get and set functions would be the best way of doing this; I was a bit concerned that my original suggestion of just passing the xml::Document would give modules too much ability to screw up the registry.

 

I wouldn't split out the name like that however - getXmlRegistryValue("/ui/state/showLightRadii") would be easier to use. I presume this was because the second text string was the name attribute to search for?

[New Horizon]

Don't want to interupt, just wanted to say that everything is looking great! I'm now able to build the editor in Linux (thanks Orbweaver), and my next project is to get doom 3 installed on linux and try working with the mod assets under linux.

 

edit: Just realized that said that I was 'not' able to build under linux. Ha ha. Corrected.

[OrbWeaver]

my next project is to get doom 3 installed on linux and try working with the mod assets under linux.

 

You probably won't be surprised to learn that CVS/SSH are massively easier under Linux as well.

[greebo]

I'm not so keen on having two UI nodes though - is it possible to merge the nodes by adding all of the children of the new one onto the existing one? This way you could use unique XPaths rather than having to search by attribute.

No problem, the search for the attribute was just an example. The list of all children of a certain type can easily be obtained by using the right XPath query. E.g. if you search for the XPath "/darkradiant/ui/toolbar" you get a list of all toolbars that are children of the node(s) under , regardless if there are one, two or hundred separate nodes below . In the above example this query would return a xml::NodeList with 2 Nodes.

 

The actual structure of the in-memory XML tree is hidden from the rest of the world anyway. Unless there is a convenient method in libxml2 for actually merging the two elements into each other, I would vote for leaving it with this.

 

I agree that separate get and set functions would be the best way of doing this; I was a bit concerned that my original suggestion of just passing the xml::Document would give modules too much ability to screw up the registry.

 

I wouldn't split out the name like that however - getXmlRegistryValue("/ui/state/showLightRadii") would be easier to use. I presume this was because the second text string was the name attribute to search for?

Yes, I thought it would be easier to use if the name was kept separate from the "class", where is located. This depends on how the key/values are stored within the XML tree anyway - but thinking about this I agree that a single XPath will probably be easier to address the settings.

[OrbWeaver]

No problem, the search for the attribute was just an example. The list of all children of a certain type can easily be obtained by using the right XPath query. E.g. if you search for the XPath "/darkradiant/ui/toolbar" you get a list of all toolbars that are children of the <ui> node(s) under <darkradiant>, regardless if there are one, two or hundred separate <ui> nodes below <darkradiant>. In the above example this query would return a xml::NodeList with 2 Nodes.

 

The actual structure of the in-memory XML tree is hidden from the rest of the world anyway. Unless there is a convenient method in libxml2 for actually merging the two <ui> elements into each other, I would vote for leaving it with this.

 

OK, that's fine. I thought if you had multiples you would have to use "/darkradiant/ui//toolbar", but if it works OK without having to explicitly use this then that shouldn't be a problem.

 

Yes, I thought it would be easier to use if the name was kept separate from the "class", where <name> is located. This depends on how the key/values are stored within the XML tree anyway - but thinking about this I agree that a single XPath will probably be easier to address the settings.

 

I think a single path is more logical, it is what you would use when accessing keys in the Windows Registry, for example.

[greebo]

Ok, the basic XMLRegistry system is working and committed now. The "show all light volumes button" is already using the new system.

 

edit: The code is now cleaned up and documented. Next step would be trying to port some of the other globals into the registry and see what problems arise or which interface functions are missing. But before that I will wait till you had a look at the code, in case I made some mistakes.

[OrbWeaver]

Looks fine so far - just a couple of things I noticed:

 

- The "show all light volumes" button seems to be on by default; this should either be off by default or persistent.

- I think getXmlRegistry() and setXmlRegistry are somewhat confusing method names, as they suggest that the entire registry is being get/set. Perhaps setProperty()/getProperty() would be more intuitive?

 

Making good progress though, once the globals are ported over we can look at some kind of persistence - perhaps dumping out the changed user.xml to the user's home folder on exit, with the principle that the user-local copy will always override the system default version (like T3Ed's INI files).

[greebo]

Looks fine so far - just a couple of things I noticed:

 

- The "show all light volumes" button seems to be on by default; this should either be off by default or persistent.

- I think getXmlRegistry() and setXmlRegistry are somewhat confusing method names, as they suggest that the entire registry is being get/set. Perhaps setProperty()/getProperty() would be more intuitive?

 

Making good progress though, once the globals are ported over we can look at some kind of persistence - perhaps dumping out the changed user.xml to the user's home folder on exit, with the principle that the user-local copy will always override the system default version (like T3Ed's INI files).

Yes, the button is currently on by default, I will change that in the next commit (this was a "leftover" from testing the globals).

 

I agree that the naming of the functions is to be changed to setProperty() or setValue(). I also thought about changing the Globalradiant()-interface to the XMLRegistry, so that the actual pointer to the xmlRegistry instance is returned (instead of translating the functions). This is easier and the plugins get access to the whole functionality of the class.

 

I will write a method that saves a specified node and all its children (e.g. "globals/state") to a file. On the next startup this can easily be loaded before the default values (duplicate nodes have their priority set in the order in which they were imported, imported first >> top priority). Unless we want the order to change, it's just the getProperty() method that has to be adapted.

[OrbWeaver]

I agree that the naming of the functions is to be changed to setProperty() or setValue(). I also thought about changing the Globalradiant()-interface to the XMLRegistry, so that the actual pointer to the xmlRegistry instance is returned (instead of translating the functions). This is easier and the plugins get access to the whole functionality of the class.

 

Returning the Registry object sounds like a good idea, since it doesn't require changes in the XMLRegistry interface to be duplicated on the GlobalRadiant module interface.

 

I will write a method that saves a specified node and all its children (e.g. "globals/state") to a file. On the next startup this can easily be loaded before the default values (duplicate nodes have their priority set in the order in which they were imported, imported first >> top priority). Unless we want the order to change, it's just the getProperty() method that has to be adapted.

 

I would tend to assume that later imports would override the earlier ones, but I don't suppose there is much difference either way as long as the behaviour is known.

[greebo]

Just wanted to drop a note here, that I'm still working on the XML library. I could hardly make some progress as my free time was too fragmented :-), but I could go some small steps forward this evening.

 

I reorganised the interface of XMLRegistry making the naming hopefully more intuitive. Newly imported XML files are internally added "on top" of the existing XML data, so that keys imported at a later time during startup actually overwrite previously existing keys.

 

I extended the functionality of the import method, so that one can specify at which point in the XML tree the keys are imported (the file becomes a child of the specified node, defaults to TOPLEVEL_NODE of course).

 

More to come, stay tuned ;-)

[OrbWeaver]

Sounds interesting. I wouldn't go overboard with the file loading functionality however, I can't imagine we are going to want to load more than 3 files - game.xml, user.xml (system default) and user.xml (user's own version)

[greebo]

A thing that will definitely need a bit of tweaking is the actual XML hierarchy, i.e. what is stored at which place.

 

At the moment I have the top node with two children: and , each of which may have several substructures like /darkradiant/ui/toolbar or /darkradiant/globals/ui/showAllLightRadii. I think it makes sense to add another node named which stores all the information from the doom3.game file and other game-specific things. Overall, I think this will grow and evolve over time.

 

I have also implemented an export function that allows to export a specific node (and all its children) into a file. This way we can save a whole tree of user settings and load them again into the right place at DR startup. I tested this with the showAllLightRadii switch and this works fine.

[OrbWeaver]

At the moment I have the top node <darkradiant> with two children: <ui> and <globals>, each of which may have several substructures like /darkradiant/ui/toolbar or /darkradiant/globals/ui/showAllLightRadii. I think it makes sense to add another node named <game> which stores all the information from the doom3.game file and other game-specific things. Overall, I think this will grow and evolve over time.

 

<game> is definitely needed, I think the other should be <user>.

 

Under <user> would be <ui>, and any other user-related values which were not related to the UI (I can't think of any at the moment, but they may happen in future).

 

What are you currently putting under <globals>? I would put the showAllLightRadii under <ui> since it is purely a UI function.

[greebo]

Don't know if we need to distinguish between "static" ui settings and ui-specific things that should be saved, like the state of the showAllLightRadii button.

 

Dropping the globals would indeed make sense, as everything stored in the registry is global by default... maybe we should create a node like "ui/state" where all ui user settings go?

 

User settings that override default settings need to have the same path, otherwise we would have to write additional code that queries both (separate) paths.

[OrbWeaver]

Don't know if we need to distinguish between "static" ui settings and ui-specific things that should be saved, like the state of the showAllLightRadii button.

Dropping the globals would indeed make sense, as everything stored in the registry is global by default... maybe we should create a node like "ui/state" where all ui user settings go?

 

I don't think such a distinction is necessary. All UI settings are essentially "state", whether that state is changed with a toolbar toggle or can only be accessed by editing the XML file.

 

User settings that override default settings need to have the same path, otherwise we would have to write additional code that queries both (separate) paths.

 

Yep - I was imagining that the user.xml in the user's directory would be identical to the version in the install directory, just with updated values.

[greebo]

Well then, I'll do it that way. I will do some tests the next few days and the commit will hopefully follow on the weekend.

[greebo]

Ok, the changes are committed now, I also changed the ToolbarCreator to use the XMLRegistry (ui.xml has been embedded into the user.xml).

 

User settings (at the moment just the state of the ShowAllLightRadii, which is off by default now) are saved at DarkRadiant exit and reloaded on startup.

 

The XMLRegistry interface for plugins has been changed as well, so that all plugins can access the whole registry by using GlobalRadiant.registry.get("") or any other function that the registry class provides.

 

I also updated the documentation of XMLRegistry, but it's quite straight forward to use, I think.