Adding a default value to ConfigFile::getSetting

[SpaceDude]

I realise the ConfigFile is just a helper class that shouldn't be of great concern but I propose a very simple change that will offer quite a significant improvement in my view. Let me describe the problem:

I use the ConfigFile class to store settings for my game because it is quite convenient. Saving settings goes something like this:

Code: Select all

ofstream ConfigFile("sound.cfg");
ConfigFile << "[General]" << endl;
ConfigFile << "Volume=" << m_Volume << endl;

And load settings like this:

Code: Select all

ConfigFile ConfigFile;
ConfigFile.load("sound.cfg");
m_Volume = StringConverter::parseReal(ConfigFile.getSetting("Volume", "General"))

With this setup, I can add new variables to be saved and loaded with just 2 lines of code. The problems occur when the sounds.cfg file doesn't contain certain settings. getSetting always returns an empty string, and if you parse that as a real it converts to 0. In the case of the volume, I don't want to volume to be set to 0 if the setting doesn't exist. I want it to be set to a more sensible default value of say 1. People launch the game and complain there is no sound, simple because the volume has been set to 0 by default. My current way around it is to do this:

Code: Select all

String Volume = ConfigFile.getSetting("Volume", "General");
if (!Volume.empty())
	m_Volume = StringConverter::parseReal(Volume);
else
	m_Volume = 1.0;

Ok it works but now I have 5 lines of code for each variable I want to load. I suggest we add a 3rd parameter to the getSetting function which will be returned in the case the setting is not found in the config file, so that the above code could be replaced with:

Code: Select all

m_Volume = StringConverter::parseReal(ConfigFile.getSetting("Volume", "General", StringConverter::toString(m_Volume)))

The change to the ConfigFile is trivial:

OgreConfigFile.h
String getSetting(const String& key, const String& section = StringUtil::BLANK, const String& default = StringUtil::BLANK) const;

OgreConfigFile.cpp
String ConfigFile::getSetting(const String& key, const String& section, const String& default) const
{

SettingsBySection::const_iterator seci = mSettings.find(section);
if (seci == mSettings.end())
{
return default;
}
....


The changes are highlighted in bold.

[sinbad]

Yep, a sensible and simple addition, I'll slap it in right now.

[SpaceDude]

Great thanks!

[tdev]

good addition! I slightly improved the FileConfig Class:
- Saving possible when loaded directly (not using resource system)
- added Type Wrappers, so you can easily store for example a Vector3 Value in it without calling stringconverter yourself.

its currently optimized for easy usage, so everything in one Header.

how you could use the code below:

Code: Select all

Ogre::ImprovedConfigFile cfg;
cfg.load("Game.cfg", "=:\t", false);
cfg.setSetting("test1", Ogre::Vector3(43.345,1,1.4),"test");
cfg.setSetting("test2", (Ogre::Real)0.1234);
// get them again to show that it works ;)
Ogre::Vector3 v = cfg.getSettingVector3("test1", "test");
Ogre::Real test2 = cfg.getSettingReal("test2");
cfg.save();

here's the code:

Code: Select all

//improved Ogre::ConfigFile thomas {AT} thomasfischer {DOT} biz 3/2008
#ifndef IMPROVEDCONFIGFILE_H_
#define IMPROVEDCONFIGFILE_H_

#include <OgrePrerequisites.h>

namespace Ogre
{

class ImprovedConfigFile : public ConfigFile
{
public:
	ImprovedConfigFile() : separators(), filename()
	{
		ConfigFile::ConfigFile();
	}
	
	~ImprovedConfigFile()
	{
	}
    
	// note: saving is only supported for direct loaded files atm!
	void load(const String& filename, const String& separators, bool trimWhitespace)
    {
		this->separators = separators;
		this->filename = filename;
		ConfigFile::load(filename, separators, trimWhitespace);
    }
	
	bool ImprovedConfigFile::save()
	{
		if(!filename.length())
		{
			OGRE_EXCEPT(Exception::ERR_INTERNAL_ERROR, "Saving of the configuration File is only allowed when the configuration was not loaded using the resource system!", "ImprovedConfigFile::save");
			return false;
		}
		FILE *f = fopen(filename.c_str(), "w");
		if(!f)
		{
			OGRE_EXCEPT(Exception::ERR_FILE_NOT_FOUND, "Cannot open File '"+filename+"' for writing.", "ImprovedConfigFile::save");
			return false;
		}

		SettingsBySection::iterator secIt;
		for(secIt = mSettings.begin(); secIt!=mSettings.end(); secIt++)
		{
			if(secIt->first.size() > 0)
				fprintf(f, "[%s]\n", secIt->first.c_str());
			SettingsMultiMap::iterator setIt;
			for(setIt = secIt->second->begin(); setIt!=secIt->second->end(); setIt++)
			{
				fprintf(f, "%s%c%s\n", setIt->first.c_str(), separators[0], setIt->second.c_str());
			}
			
		}
		fclose(f);
		return true;
	}

	void setSetting(String &key, String &value, String section = StringUtil::BLANK)
	{
		SettingsMultiMap *set = mSettings[section];
		if(!set)
		{
			// new section
			set = new SettingsMultiMap();
			mSettings[section] = set;
		}
		if(set->count(key))
			// known key, delete old first
			set->erase(key);
        // add key
		set->insert(std::multimap<String, String>::value_type(key, value));
	}

	// type specific implementations
	Radian getSettingRadian(String key, String section = StringUtil::BLANK) { return StringConverter::parseAngle(getSetting(key, section));	}
	void setSetting(String key, Radian value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section);	}

	bool getSettingBool(String key, String section = StringUtil::BLANK) { return StringConverter::parseBool(getSetting(key, section)); }
	void setSetting(String key, bool value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	Real getSettingReal(String key, String section = StringUtil::BLANK) { return StringConverter::parseReal(getSetting(key, section)); }
	void setSetting(String key, Real value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	int getSettingInt(String key, String section = StringUtil::BLANK) { return StringConverter::parseInt(getSetting(key, section)); }
	void setSetting(String key, int value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	unsigned int getSettingUnsignedInt(String key, String section = StringUtil::BLANK) { return StringConverter::parseUnsignedInt(getSetting(key, section)); }
	void setSetting(String key, unsigned int value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	long getSettingLong(String key, String section = StringUtil::BLANK) { return StringConverter::parseLong(getSetting(key, section)); }
	void setSetting(String key, long value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	unsigned long getSettingUnsignedLong(String key, String section = StringUtil::BLANK) { return StringConverter::parseUnsignedLong(getSetting(key, section)); }
	void setSetting(String key, unsigned long value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	Vector3 getSettingVector3(String key, String section = StringUtil::BLANK) { return StringConverter::parseVector3(getSetting(key, section)); }
	void setSetting(String key, Vector3 value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	Matrix3 getSettingMatrix3(String key, String section = StringUtil::BLANK) { return StringConverter::parseMatrix3(getSetting(key, section)); }
	void setSetting(String key, Matrix3 value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	Matrix4 getSettingMatrix4(String key, String section = StringUtil::BLANK) { return StringConverter::parseMatrix4(getSetting(key, section)); }
	void setSetting(String key, Matrix4 value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	Quaternion getSettingQuaternion(String key, String section = StringUtil::BLANK) { return StringConverter::parseQuaternion(getSetting(key, section)); }
	void setSetting(String key, Quaternion value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	ColourValue getSettingColorValue(String key, String section = StringUtil::BLANK) { return StringConverter::parseColourValue(getSetting(key, section)); }
	void setSetting(String key, ColourValue value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

	StringVector getSettingStringVector(String key, String section = StringUtil::BLANK) { return StringConverter::parseStringVector(getSetting(key, section)); }
	void setSetting(String key, StringVector value, String section = StringUtil::BLANK) { setSetting(key, StringConverter::toString(value), section); }

protected:
	String separators;
	String filename;
};

};
#endif

useful? not useful?
should i submit a patch for the ConfigFile class? (using Filestreams then?)

[SpaceDude]

Those look like sensible additions, although it would be nice to have just one "getSetting" function which returns the desired type rather than "getSettingXxx". One way to do that is to determine the type from the default parameter, for example:

Code: Select all

bool getSetting(String key, String section, bool default);
String getSetting(String key, String section, String default);
Real getSetting(String key, String section, Real default);
int getSetting(String key, String section, int default);

which can then be used as:

Code: Select all

bool foo = config.getSetting("hey", "man", true);
Real bar = config.getSetting("this", "iscool", 2.0);

That's how the GetPot library works (a more advanced version of ogre's ConfigFile): http://getpot.sourceforge.net/node9.html

Unfortunately then the 2nd parameter cannot default to blank. Unless it is moved to the end which would be ugly.

Update: To avoid the problem of the second parameter you could define the following function:

Code: Select all

template<typename T>
T getSetting(String key, T default)
{
    return getSetting(key, StringUtil::BLANK, default);
}

hmm... scratch that, it's nasty and would conflict with "getSetting(String key, String section)"

[tdev]

Code: Select all

bool getSetting(String key, String section, bool default);
String getSetting(String key, String section, String default);
Real getSetting(String key, String section, Real default);
int getSetting(String key, String section, int default);

Thats a nice solution to use the default value to create different function signatures

About the group optional problem: why not simply give the default value a default value like that

Code: Select all

Real getSetting(String key, String section=StringUtil::BLANK, Real default=0); 

[CABAListic]

Because then the compiler can no longer deduce which of the overloaded functions you mean.
If you want NOT to give a default value, then you have to templatise the function and tell the type you want instead, i. e. you'd then call

Code: Select all

getSetting<Real>(key, section); 

. Actually, this is more robust, anyway, as values for default arguments can be ambiguous, but it's slightly more complicated because StringConverter is not templatised. Using boost::lexical_cast instead would make this quite simple

[tdev]

Because then the compiler can no longer deduce which of the overloaded functions you mean.

oh, indeed. :\
no simple solution then.

[SpaceDude]

Because then the compiler can no longer deduce which of the overloaded functions you mean.
If you want NOT to give a default value, then you have to templatise the function and tell the type you want instead, i. e. you'd then call

Code: Select all

getSetting<Real>(key, section); 

. Actually, this is more robust, anyway, as values for default arguments can be ambiguous, but it's slightly more complicated because StringConverter is not templatised. Using boost::lexical_cast instead would make this quite simple

Thanks for the lexical_cast suggestion, I've extended the class in the following way and it works great (see code below). I'm now able to type something like:

Code: Select all

Real val = getSetting("Value", "General", 1.0);

And in the face of ambiguity you can always explicitly specify the type. Unfortunately it's not suitable to become part of Ogre since it depends on boost, but others may be interested in it.

Code: Select all

class CustomConfigFile : public ConfigFile
{
public:
	String getSetting(const String& key, const String& section = StringUtil::BLANK) const
	{
		return ConfigFile::getSetting(key, section);
	}
	template <typename T>
	T getSetting(const String& key, const String& section, T DefaultValue) const
	{
		SettingsBySection::const_iterator seci = mSettings.find(section);
		if (seci == mSettings.end())
		{
			return DefaultValue;
		}
		else
		{
			SettingsMultiMap::const_iterator i = seci->second->find(key);
			if (i == seci->second->end())
			{
				return DefaultValue;
			}
			else
			{
				try
				{
					return typename lexical_cast<T>(i->second);
				}
				catch(...)
				{
					return DefaultValue;
				}
			}
		}
	}
};

[lf3thn4d]

Hi, I found a bug with the default value not returned when the section is present but key is not.

Since it's just a single line fix, i'm lazy to do a patch.

To fix this:

Code: Select all

        SettingsBySection::const_iterator seci = mSettings.find(section);
        if (seci == mSettings.end())
        {
            return defaultValue;
        }
        else
        {
            SettingsMultiMap::const_iterator i = seci->second->find(key);
            if (i == seci->second->end())
            {
                return defaultValue; <<----- this line fix. was empty string before.
            }
            else
            {
                return i->second;
            }
        }

[spookyboo]

I assume that the patch doesn't work if the .cfg file doesn't exist at all. I suggest to extend it , so when .cfg file doesn't exist, only default settings are returned.

[lf3thn4d]

Well, if file doesn't exist, the map would have been empty. Hence it would return the default value anyways. So I don't see any problem there. This one line fix is all that is needed.

[Invader Zim]

I find it very un intuitive that the default value is only returned if the config-section doe snot exist, but a blank string is returned if the section exists, but the setting does not. Is that really intentional? If it is, then I suggest the documentation should reflect it (but I guess it's the code that needs changing)

Code: Select all

    String ConfigFile::getSetting(const String& key, const String& section, const String& defaultValue) const
    {
        
        SettingsBySection::const_iterator seci = mSettings.find(section);
        if (seci == mSettings.end())
        {
            return defaultValue;
        }
        else
        {
            SettingsMultiMap::const_iterator i = seci->second->find(key);
            if (i == seci->second->end())
            {
                return StringUtil::BLANK;  // Why?
            }
            else
            {
                return i->second;
            }
        }
    }

[lf3thn4d]

Yes, it's a bug. I've even provided the fix for it on my previous post. But nobody seemed to care about fixing it though. :/ I'm too lazy to submit a patch to the tracker atm. Maybe you can do it?

[syedhs]

Okay I just hit the same bug, as pointed by the earlier postings. Okay here is the patch (very simple) :-

Code: Select all

Index: OgreConfigFile.cpp
===================================================================
--- OgreConfigFile.cpp	(revision 8537)
+++ OgreConfigFile.cpp	(working copy)
@@ -168,7 +168,7 @@
             SettingsMultiMap::const_iterator i = seci->second->find(key);
             if (i == seci->second->end())
             {
-                return StringUtil::BLANK;
+                return defaultValue;
             }
             else
             {

[sinbad]

Applied.