Global settings singleton

[mspraggs]

Yes, you read correctly, I'm suggesting a singleton.

Basically it'd be nice to have all the stuff in Constants.hpp in some class so that they can be modified easily when the game is started up.

However, to avoid any constants being changed after the game is started, I propose some mechanism whereby the state of the singleton can be locked and no further changes made. This would remove the temptation to have some global variables that can be changed during the game and potentially introduce bugs.

What do you think?

[Fyll]

Sounds good (I think I did this for a previous game, but it ended up a mess due to my younger, unsophisticated self). My main problem would be the fiddlyness of adding "Settings::instance()." in front of every constant.

Would this be for every constant, or just those which can be set at start-up (e.g. would things like the amount of points for clearing a room be constant, or a setting)?

[mspraggs]

I was thinking everything in Constants.hpp that is declared const.

What I think I'll do is have a std::unordered_map member pairing std::strings to the settings values. That way arbitrary setting names can be paired to values, making it more extensible.

[Fyll]

So you'd just call Settings::instance().getConst("FULLSCREEN") for example? That would save on having to define a new function for each one.

I suppose then the question is what naming convention do we use? Do we continue to do all caps and underscores, or move to no caps and spaces or what?

Also, if you're not against it, could we have an inline Settings& getConst(const std::string& name) { return Settings::instance().getConst(name); } or the like to reduce the bulkyness of the code? I'm pretty sure I remember something about this sort of thing being bad, but that might have just been because it was macros back then.

[mspraggs]

Yep, or even just overloading operator[] and making the lookup easier to type.

I don't mind the way it is now, but if you or David want to change it, that's fine too. As long as we stick to a convention, I'm easy.

An inline function is also fine. The Config instance is of course global anyway, so another global function on top of this won't make any difference. My previous complaint was with macros, as they obfuscate the code during debugging. Another approach, if you know you're going use it a lot in a particular scope, is to just assign it to a reference at the start of the scope.

[mspraggs]

Ok I've implemented this, though I haven't applied it yet. Because there are varying types in Constants.hpp, I kind of had to steal someone else's idea and create a templated wrapper class for the stored values that allocates a static unordered_map for each type that someone wants to store. Because this is all internal stuff that nobody should have to look at without a few stiff drinks, I've chucked it inside a namespace.

I was thinking that default values could be set in the Config constructor. I haven't moved anything in Constants.hpp over to the new class because frankly it's ten to twelve and I'm unlikely to finish it before bed.

As requested there's an inline function to make getting the constants easier. It's templated (various return types etc.), just so you're aware.

[Fyll]

If it's a singleton the constructor will be private/protected, surely? Would it not have to be in some sort of Config::init() function (ha ha! I was right all along!)? :P

Also, an added advantage of this is that we would only be editing a .cpp when we want to change the constants, rather than having to recompile everything due to changing the .hpp! Hooray!

[mspraggs]

Well I thought you could have a default constructor where the default setttings are initialized. Then it's open as to whether the user changes settings from their default values by using the set function in Config. The default constructor could go in a source file, so this wouldn't be an issue.

Another thing that may be nice would be to have a readFromFile function on Config, so some values could be overridden at startup by passing in a text file. This would completely avoid the need for recompiling from source etc. Just a thought.

[Fyll]

But you can't pass anything into the constructor as it's private, and if you have to edit the file to change things, aren't we back where we started? I still think it'd be better to have a lock function, then you can pass in as much as you want (one thing at a time) when parsing the command line options or reading a file, then you'd call Config::lock() before starting the rest of the code.

[DivFord]

Would a friend function be useful here? http://www.cplusplus.com/doc/tutorial/inheritance/

[mspraggs]

There should be a lock function. I think I added one in.

The idea would be to put all defaults in the default constructor, then use the set member function to change any as necessary when the game starts up, then call lock() when everthing's set up. I think I put a basic use case in main().

The configuration file parsing was just an idea. We don't have to use it.

EDIT: This is my basic use case in main():

// Testing the Config class. Does nothing at this stage.
auto& config = Config::instance();
config.set("BLAH", 123);
config.lock();
std::cout << config.get<int>("BLAH") << std::endl;
config.set("BLAH", 543); // Should print a warning

In the first step, when instance() is called, the default constructor for Config is called, and any default values in settings are set. If any of these defaults need to be overridded, then the set method can be used, as in the example above.

[Fyll]

Hmm. We seem to be advocating the same thing. I must have misinterpreted what you said.

Still, all's good then! (I had noticed the warning popping up and just assumed it was something you were still working on),

[DivFord]

Anyone else getting a segfault on quitting? Seems to be related to this.

[Fyll]

I'm not. How are you quitting (escape, walking off of the left, pressing the X)?

[DivFord]

Happens for both. I'm not passing any arguments. Should I be?

[Fyll]

I'm fine without any arguments. What makes you say it's related to this?

[mspraggs]

Could you perhaps open a new issue and paste one of those glorious tracebacks like we saw earlier today?

[DivFord]

It mentions an unordered map, and the only one I could think of that was recently added was for this.

EDIT: Sorry, traceback is in a new issue instead. Missed that bit.

[mspraggs]

Okay. By the looks of it it's crashing in Value::accessData. I have a feeling that the offset (+ 1120) refers to the instruction offset in the assembly listing, which isn't very helpful here. Do you have gdb installed? Try running the gdb command in the terminal.

If you do have gdb, try this:

1. gdb -q ./game
2. type "run" and press enter.
3. Play a bit, quit the game, hopefully you'll get a stack trace.

Hopefully the stack trace produced by gdb should tell us which line it's failing on.

[DivFord]

Is this what you're after? The program failed to actually quit this time, so I had to force quit it.

Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000008 std::1::hash_table<std::1::pair<detail::ValueWrapper const*, int>, std::1::unordered_map_hasher<detail::ValueWrapper const*, int, std::1::hash<detail::ValueWrapper const>, true>, std::1::unordered_map_equal<detail::ValueWrapper const, int, std::1::equal_to<detail::ValueWrapper const>, true>, std::1::allocator<std::1::pair<detail::ValueWrapper const, int> > >::find<detail::ValueWrapper const*> (this=0x100049e90, k=@0x7fff5fbff438) at hash_table:1593 1593 size_t chash = constrain_hash(hash, __bc);

[mspraggs]

That looks like the information for the top-most stack frame. If you look at the first few frames of the trace you posted before:

0 game 0x000000010b1c75c0 std::__1::__hash_iterator, void>> std::__1::__hash_table, std::__1::__unordered_map_hasher, true>, std::__1::__unordered_map_equal, true>, std::__1::allocator > >::find(detail::ValueWrapper const* const&) + 464
1 game 0x000000010b1c730b unsigned long std::__1::__hash_table, std::__1::__unordered_map_hasher, true>, std::__1::__unordered_map_equal, true>, std::__1::allocator > >::__erase_unique(detail::ValueWrapper const* const&) + 59
2 game 0x000000010b1c6a30 int const& detail::ValueWrapper::accessData(int const&, detail::ValueWrapper::Action) const + 1120
3 game 0x000000010b1cce74 void detail::ValueWrapper::deleteData() const + 36

The first two frames relate to functions in the standard library. Frame 2 relates to our code. If you can get the same information that you've just posted, but for frame 2, then we may be able to get the line in Config.hpp that's causing it to crash.

[mspraggs]

I'm going to try compiling with clang, just to see whether there's something in the clang version of the standard library that causes things to go awry.

EDIT: Nope, no difference.

[DivFord]

No luck getting the other frames. It just freezes.

[mspraggs]

Are you using gdb or some other debugger?

[DivFord]

Using gdb.

It prints "Freedom!" before the bad access message. Is that relevant?

[mspraggs]

Hmm. Yes and no. If you look in main.cpp, you notice at the bottom that this where that message is printed. However, the Game instance hasn't been destroyed at this point. If you look at the original stack trace you posted, you may notice that the exception happens inside ~Value, which in turn is inside ~Config. So these objects are being destroyed when the game instance is destroyed at the end of main. For some reason this causes an exception.

I might run this through valgrind and see if I get any memory allocation errors.

[mspraggs]

Low and behold: memory errors, according to valgrind.

I'm not entirely sure what caused the error. I have feeling it's due to the static variable inside the ValueWrapper::accessData class, but I can't be more specific. Anyway I realised I could significantly simplify the code inside the detail namespace in Config.hpp, and in the process this seems to have fixed the issue.

[DivFord]

That's fixed my problem. Thanks!

[mspraggs]

I have now gone through the code and changed all references to global constants to use the Config instance.

There were some teething problems relating to the initialisation of the various lists of things - gun list, block list etc. The Config class works by using a static member in ValueWrapper. This static member is initialised before main is called. However, since this member variable and the various lists are initialised in different translation units (source files), it's impossible to predict which will be initialised first. Since the latter depend on the former, the lists must be initialised after the Config instance is set up.

To sidestep this issue I had to butcher the code somewhat and create functions that initialise the lists within the main function. I don't find this solution satisfying at all, as I had to use the extern keyword, which generally causes me some distress. It may make more sense long-term to move the lists of things into the Config instance too, and have the setup functions set the lists within the Config instance.