Adding a default value to ConfigFile::getSetting

Discussion area about developing or extending OGRE, adding plugins for it or building applications on it. No newbie questions please, use the Help forum for that.
Post Reply
User avatar
SpaceDude
Bronze Sponsor
Bronze Sponsor
Posts: 822
Joined: Thu Feb 02, 2006 1:49 pm
Location: Nottingham, UK
Contact:

Adding a default value to ConfigFile::getSetting

Post by SpaceDude » Wed Mar 19, 2008 10:47 am

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.
0 x

User avatar
sinbad
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 19261
Joined: Sun Oct 06, 2002 11:19 pm
Location: Guernsey, Channel Islands
Contact:

Post by sinbad » Wed Mar 19, 2008 6:06 pm

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

User avatar
SpaceDude
Bronze Sponsor
Bronze Sponsor
Posts: 822
Joined: Thu Feb 02, 2006 1:49 pm
Location: Nottingham, UK
Contact:

Post by SpaceDude » Wed Mar 19, 2008 9:37 pm

Great thanks!
0 x

User avatar
tdev
Silver Sponsor
Silver Sponsor
Posts: 244
Joined: Thu Apr 12, 2007 9:21 pm
Location: Germany

Post by tdev » Thu Mar 20, 2008 3:14 pm

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?)
0 x

User avatar
SpaceDude
Bronze Sponsor
Bronze Sponsor
Posts: 822
Joined: Thu Feb 02, 2006 1:49 pm
Location: Nottingham, UK
Contact:

Post by SpaceDude » Thu Mar 20, 2008 4:51 pm

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)"
0 x

User avatar
tdev
Silver Sponsor
Silver Sponsor
Posts: 244
Joined: Thu Apr 12, 2007 9:21 pm
Location: Germany

Post by tdev » Thu Mar 20, 2008 5:45 pm

SpaceDude wrote:

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); 
0 x

CABAListic
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 2903
Joined: Thu Jan 18, 2007 2:48 pm
Contact:

Post by CABAListic » Thu Mar 20, 2008 6:54 pm

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 ;)
0 x

User avatar
tdev
Silver Sponsor
Silver Sponsor
Posts: 244
Joined: Thu Apr 12, 2007 9:21 pm
Location: Germany

Post by tdev » Sat Mar 22, 2008 1:50 am

CABAListic wrote:Because then the compiler can no longer deduce which of the overloaded functions you mean.
oh, indeed. :\
no simple solution then.
0 x

User avatar
SpaceDude
Bronze Sponsor
Bronze Sponsor
Posts: 822
Joined: Thu Feb 02, 2006 1:49 pm
Location: Nottingham, UK
Contact:

Post by SpaceDude » Mon Apr 14, 2008 11:51 am

CABAListic wrote: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;
				}
			}
		}
	}
};
0 x

User avatar
lf3thn4d
Orc
Posts: 478
Joined: Mon Apr 10, 2006 9:12 pm

Post by lf3thn4d » Tue Nov 04, 2008 9:16 am

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. :P

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;
            }
        }
0 x

User avatar
spookyboo
Silver Sponsor
Silver Sponsor
Posts: 1140
Joined: Tue Jul 06, 2004 5:57 am
x 15
Contact:

Post by spookyboo » Tue Nov 04, 2008 10:48 am

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.
0 x

User avatar
lf3thn4d
Orc
Posts: 478
Joined: Mon Apr 10, 2006 9:12 pm

Post by lf3thn4d » Tue Nov 04, 2008 4:29 pm

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.
0 x

User avatar
Invader Zim
Halfling
Posts: 55
Joined: Fri Mar 25, 2005 11:42 am
Location: Oslo, Norway
Contact:

Re: Adding a default value to ConfigFile::getSetting

Post by Invader Zim » Sat Feb 14, 2009 11:36 am

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;
            }
        }
    }
0 x

User avatar
lf3thn4d
Orc
Posts: 478
Joined: Mon Apr 10, 2006 9:12 pm

Re: Adding a default value to ConfigFile::getSetting

Post by lf3thn4d » Sun Feb 15, 2009 2:55 pm

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? :)
0 x

User avatar
syedhs
Silver Sponsor
Silver Sponsor
Posts: 2702
Joined: Mon Aug 29, 2005 3:24 pm
Location: Kuala Lumpur, Malaysia
x 2

Re: Adding a default value to ConfigFile::getSetting

Post by syedhs » Wed Apr 22, 2009 10:46 am

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
             {
0 x
A willow deeply scarred, somebody's broken heart
And a washed-out dream
They follow the pattern of the wind, ya' see
Cause they got no place to be
That's why I'm starting with me

User avatar
sinbad
OGRE Retired Team Member
OGRE Retired Team Member
Posts: 19261
Joined: Sun Oct 06, 2002 11:19 pm
Location: Guernsey, Channel Islands
Contact:

Re: Adding a default value to ConfigFile::getSetting

Post by sinbad » Thu Apr 23, 2009 5:53 pm

Applied.
0 x

Post Reply