Combining Xml Trees

[OrbWeaver]

Cool, I'll look over the changes and hopefully start pushing some GUI stuff into the XML (such as the sizes of the GL preview widgets).

[OrbWeaver]

Looking very good so far. I successfully moved the ModelSelector preview size into user.xml, so that users can choose a different size by editing this value (up to 1.0 for the entire screen height).

 

Now that we have GlobalRadiant().registry(), do we need the "extern xmlRegistry" global variable in XMLRegistry.h? I wasn't quite sure what this variable does, but it would seem that the GlobalRadiant approach is better than a global variable.

[greebo]

Is there another possibility for the GlobalRadiant().registry() function (and other functions) to obtain the XMLRegistry instance other than a global variable that is defined in the radiant core environment?

 

If yes, we sure could join these two (although the simple xmlRegistry variable would be much shorter to type, but this is more of cosmetic nature). I'm not sure about what can be done in C++ and what not, that's why the above question may sound a bit trivial...

[OrbWeaver]

Is there another possibility for the GlobalRadiant().registry() function (and other functions) to obtain the XMLRegistry instance other than a global variable that is defined in the radiant core environment?

 

The simplest way is to have a function that returns a reference to a local static object:

 

Object& getObject() {
  static Object _objectInstance(param1, param2);
  return _objectInstance;
}

 

Then any function that needs the instance just calls getObject() and is guaranteed to get the exact same instance as every other calling function.

 

If there is a need to share the instance amongst multiple functions, then the instance can be a member of an object:

 

class Radiant {
  Object _object;
public:
  Object& getObject() {
  return _object;
  }
  void doSomethingToObject() {
  _object.doSomething();
  }
};

 

In actual fact the first technique can be used for everything, since the function doSomethingToObject could simply call "getObject().doSomething()" to access the instance, but it's a matter of style whether you want the object to be owned by the function or by the class.

 

I would tend to assume that any object returned by GlobalRadiant().getSomething() would be a member of the class that is returned by calling GlobalRadiant().

[greebo]

I would tend to assume that any object returned by GlobalRadiant().getSomething() would be a member of the class that is returned by calling GlobalRadiant().

Am I getting this right?

 

namespace xml {

class xmlRegistryWrapper {
 static XMLRegistry _xmlRegistry;
 public:
XMLRegistry* getRegistry() {
  return &_xmlRegistry;
}
}

} // namespace xml

xml::XMLRegistry* registry() {
 return xml::xmlRegistryWrapper::getRegistry();
}

and within RadiantCoreAPI:

 

  m_radiantcore.registry = &registry;

or could the RadiantCoreAPI "link" directly to the class method? Like this:

  m_radiantcore.registry = &(xml::xmlRegistryWrapper::getRegistry);

[OrbWeaver]

I think you're confusing yourself a bit. It doesn't need to be that complex.

 

In radiant/plugin.cpp you have a function registry() which returns an xml::XMLRegistryPtr that points to a global shared XMLRegistry instance. Instead of returning a pointer to this instance, write the registry() function thus:

 

xml::XMLRegistry& registry() {
  static xml::XMLRegistry _registryInstance;
  return _registryInstance;
}

 

Note I am using a reference rather than a pointer so you don't have to use "->" to operate on it.

 

From hereon in, any time you would have written:

 

xmlRegistry->doSomething(); // global variable

 

write

 

GlobalRadiant().registry().doSomething(); // accessed via function

 

Or if that gets too cumbersome because you need to call a lot of methods in succession:

 

xml::XMLRegistry& reg = GlobalRadiant().registry();
reg.doSomething();
reg.doSomethingElse();

 

You may be getting confused by all of the function pointers in _QERFuncTable_1. This is an ugly old way of representing an interface, instead this should be an Abstract Base Class with virtual methods that are overridden by an implementing subclass (like the ShaderSystem interface in ishaders.h, for example).

[greebo]

I think you're confusing yourself a bit. It doesn't need to be that complex.

 

xml::XMLRegistry& registry() {
  static xml::XMLRegistry _registryInstance;
  return _registryInstance;
}

Yes, I realised this as soon as I read through my above post and thought of the same solution (except for passing a reference)

 

You may be getting confused by all of the function pointers in _QERFuncTable_1. This is an ugly old way of representing an interface, instead this should be an Abstract Base Class with virtual methods that are overridden by an implementing subclass (like the ShaderSystem interface in ishaders.h, for example).

No, the pointers are not the problem, it didn't take me long to figure this out - it's those template cascades that really give me the creeps...

 

How would such an implementation of an abstract base class access radiant's core assets like the XMLRegistry? Would a reference need to be passed to the implementation by the plugin loading system?

[OrbWeaver]

How would such an implementation of an abstract base class access radiant's core assets like the XMLRegistry? Would a reference need to be passed to the implementation by the plugin loading system?

 

The first difference is that the interface would be different. Instead of

 

struct _QERFuncTable_1
{
 const char* (*getEnginePath)();
 const char* (*getGameToolsPath)();
 xml::XMLRegistry* (*registry)();
...

 

You would have

 

struct _QERFuncTable_1
{
 virtual const char* getEnginePath() = 0;
 virtual const char* getGameToolsPath() = 0;
 virtual xml::XMLRegistry& registry() = 0;
...

 

Although it would preferable be called something slightly less asinine than "_QERFuncTable_1".

 

Then, instead of a RadiantCoreAPI object (radiant/plugin.cpp) with an instance of the func table class, the RadiantCoreAPI would inherit directly from the interface and provide implementations for its functions. It would also need a getTable() function returning a _QERFuncTable_1 pointer, just like the current one, except rather than returning a contained object it would simply return itself.

 

class RadiantCoreAPI
: public _QERFuncTable1
{
 xml::Registry& registry() {
// return the static registry stuff
 }
 ...
 _QERFuncTable_1* getTable() {
return this;
 }
...

[greebo]

Ok, this sounds simpler than I thought. The radiant source code doesn't look like a good place to learn such stuff by code-reading. <_>

 

One question though: can the class RadiantCoreAPI inherit from a struct or is this a typo?

 

Nevertheless, I've committed all the changes now, including the removal of the texture menu.

 

One note on the registry behaviour: You may already have noticed that whenever the install/user.xml changes, one has to delete the user.xml in the user settings folder, otherwise the changes would get buried under the user settings (they are added above the default values).

[OrbWeaver]

Ok, this sounds simpler than I thought. The radiant source code doesn't look like a good place to learn such stuff by code-reading.

 

No it isn't - it is half C and half C++, and the parts that are C++ tend be complex and ugly.

 

One question though: can the class RadiantCoreAPI inherit from a struct or is this a typo?

 

Yes, a struct is just another name for a class, except if you declare something as a struct all the members are public by default (so you don't need a "public:" modifier).

 

One note on the registry behaviour: You may already have noticed that whenever the install/user.xml changes, one has to delete the user.xml in the user settings folder, otherwise the changes would get buried under the user settings (they are added above the default values).

 

Yes I did notice that. Will there be any problems with this if users install new version of DarkRadiant with an update user.xml? It seems that new entries in the install/user.xml do take effect, even though they are not replicated to the user's local copy, so perhaps this won't be an issue.

[greebo]

Will there be any problems with this if users install new version of DarkRadiant with an update user.xml? It seems that new entries in the install/user.xml do take effect, even though they are not replicated to the user's local copy, so perhaps this won't be an issue.

I can imagine that if we add new toolbar buttons in a new DarkRadiant release the changes won't take an effect unless the user's own XML is deleted. I also thought of adding a version tag somewhere in the install/user.xml so that DarkRadiant can check whether the user.xml is possibly stemming from a previous DR install, and it can take the appropriate actions (i.e. delete the user.xml or merge it with the new settings or at least notify the user).

 

But I guess this will not be a major problem as long as we have a hand full of DR beta mappers. Till we're in the situation of having hundreds of DarkRadiant users and we plan to release a new version, we sure will come up with some solution.

 

Yes, a struct is just another name for a class, except if you declare something as a struct all the members are public by default (so you don't need a "public:" modifier).

Ah, didn't know that one! Well, I'm learning something new every day...

[OrbWeaver]

Does the registry contain the game XML settings as well as the user ones? It would be best to have a unified interface for both game and user settings, rather than having to use GlobalRadiant().registry().findXPath() for user and GlobalRadiant().getXPath() for game.

[greebo]

Yes, the game settings are already stored in the registry, you can use the registry() method for access game information from now on.

 

I did not change anything of the "old" CGame* interface, so there exist both possibilites at the moment.

 

edit: There is a debug method called registry().dump(), which outputs all XML content of the registry to the console, if you ever need it.

[OrbWeaver]

Strange, for some reason I can't access the game settings in the manner I would expect.

 

GlobalRadiant().getXPath("/game/filtersystem//filter");

 

works, but

 

GlobalRadiant().registry().findXPath("game/filtersystem//filter");

 

doesn't.

 

Am I doing something wrong? The changes so far are committed (there is now a totally useless top-level Filter menu with entries populated from the game XML file), in case you want to look at it.

[greebo]

I will take a look at it. Have you tried to use "//game/filtersystem//filter"?

[greebo]

I found the problem: the findXPath method of the registry() object didn't append the /darkradiant automatically. The changes to XMLRegistry are committed, it's working now:

 

xml::NodeList filters = GlobalRadiant().registry().findXPath("game/filtersystem//filter");

[OrbWeaver]

Thanks, I've update the filters code accordingly.

[greebo]

Before I will take a look at the code for the draggable light center, I will implement the versioning check of the user.xml file. I think I will need about two or three days (maximum) to do this. Is this fast enough for the release of DarkRadiant 0.7.0, Orbweaver?

[OrbWeaver]

Before I will take a look at the code for the draggable light center, I will implement the versioning check of the user.xml file. I think I will need about two or three days (maximum) to do this. Is this fast enough for the release of DarkRadiant 0.7.0, Orbweaver?

 

Should be fine - often I have done releases on a Sunday afternoon (after using the weekend for development).

 

Presumably it wouldn't actually matter if the feature didn't make it into 0.7, provided that when it was introduced in 0.8 it could correctly deal with the unversioned file.

[greebo]

Presumably it wouldn't actually matter if the feature didn't make it into 0.7, provided that when it was introduced in 0.8 it could correctly deal with the unversioned file.

I just wanted to post this here, when I realised you already edited this into your post

 

I will just add a version tag to the current user.xml, this should make upgrading even easier in the future.

[greebo]

The upgrade code is committed now including two upgrade paths from earlier versions. All the upgrade information is stored in the install/user.xml in some sort of a scripting language. For example, this allows the colour schemes and the state of the showAllLightRadii button to be copied into a higher user.xml version:

 

<upgradePath fromVersion="0.6.0">
	<!-- Find the currently active scheme and store its name to Memory 1 -->
	<saveAttribute name="name" xPath="/user/ui/colourschemes//scheme[@active='1']" memoryIndex="1" />
	<!-- Make sure the default schemes are removed before copying user schemes -->
	<deleteNode xPath="/user/ui/colourschemes//scheme[@name='QE3Radiant Original']" />
	<deleteNode xPath="/user/ui/colourschemes//scheme[@name='Maya/Max/Lightwave Emulation']" />
	<deleteNode xPath="/user/ui/colourschemes//scheme[@name='DarkRadiant Default']" />
	<deleteNode xPath="/user/ui/colourschemes//scheme[@name='Black & Green']" />
	<!-- Clear all active attributes in the base registry -->
	<setAttribute target="base" xPath="//user/ui/colourschemes//scheme" name="active" value="0" />
	<!-- Copy all remaining colour schemes into the new registry -->
	<copyNode fromXPath="/user/ui/colourschemes//scheme[@readonly!='1']" toXPath="user/ui/colourschemes" />
	<!-- Set the active scheme to the one that was stored in Memory 1 -->
	<setAttribute xPath="/user/ui/colourschemes//scheme[@name='{$1}']" name="active" value="1" />
	<!-- Import the state of showAllLightRadii into the new registry -->
	<copyValue fromXPath="/user/ui/showAllLightRadii" toXPath="user/ui/showAllLightRadii" />
</upgradePath>

 

I will do some testing before the next release, where I will ask some of our beta mappers for their current user.xml, so that I can test it on my machine (angua's version is already working ).

[OrbWeaver]

It looks...erm...flexible. As in a sendmail.cf kind of way...

[greebo]

I get your point, but it's not very complicated, the forum's "code formatting" doesn't help, I have to admit...

 

I didn't want to hardcode all this stuff and thought it could come in handy.

[OrbWeaver]

Well if it works, that's the important thing.

 

Is there going to be a problem with bloat? It will get quite messy if we have an extra upgradePath section for every previous version.

[greebo]

This maybe a problem if the registry gets reorganised a lot. In this case I could try and implement a stage-wise upgrade, so that an xml file with version 0.7.0 gets "converted" into 0.8.0 and then upwards till it reaches the most recent version. This shouldn't be too hard, I think, and I will take care about this when it becomes necessary .