Global variable for the world?

[neroden]

After doing quite a lot of work on simutrans code, it strikes me that it would be very useful to have an  *global variable* to
point to the world. 

karte_t* world;

It doesn't have to be a "C style global variable", but it should be effectively a global variable.  If you want to allow for "const optimization", it could be a static variable in a widely used class, with something like this:

Code Select Expand


(in simworld.h header file)
static inline const karte_t* world() { return karte_t::world; }
...
class karte_t {
...
friend int simumain(int argc, char** argv);
friend const karte_t* get_world();
private:
static karte_t* world;
static inline void karte_t::set_world(karte_t* new_world) {world = new_world;} // call only from simmain!
...
};
(in simworld.cc file)
karte_t* karte_t::world;


(Note that static and global data is guaranteed to be initialized to NULL before the program starts.  Because of how often it is used, I suggest a plain function, outside any class, with a very short name, for accessing this variable.  Save typing for programmers!)

Another possible implementation is for all classes to inherit from a single base class which provides the static "world" variable and the inline "world()" method, though this has some potential multiple inheritance pitfalls:

Code Select Expand


(in in_world.h header file)
class karte_t;

class in_world {
public:
inline const karte_t* world() {return in_world::world; }
friend int simumain(int argc, char** argv);
private:
static karte_t* world;
static inline void karte_t::set_world(karte_t* new_world) {world = new_world;} // call only from simmain!
};
(in in_world.cc file)
karte_t* in_world::world;

Effectively this is just another way of writing a global variable.

Motivation:

(1) I don't think we are ever going to want to have two worlds active at the same time.
(2) One by one, every single class is accumulating "welt" and "world" pointer variables, sometimes static, sometimes not.  Varying amounts of care are taken with these.
(3) A bunch of little functions are accumulating to set these variables, too.
(4) The world is already created once and exactly once, in simmain.cc
(5) An extremely short list of classes are used before world creation, so not very many classes need to worry about the lack of a world.  Those which do can check whether the pointer is NULL, and the pointer will be initialized to NULL if it's in a static or global variable.

I think this is a classic case for a global variable.  This could eliminate:
(1) Lots and lots of passing of "karte_t* welt" on the stack
(2) Lots of separate copies of "karte_t* welt" in memory
(3) Lots of unnecessary and useless code for programmers to read and sift through

I know that global variables are frowned upon, but this is what they're for.

If there are any serious plans to support multiple worlds at once, then this would be a bad idea.  But I think that there are no such plans.  Tell me if I'm wrong...

[Ters]

I have been thinking of this as well, but my general (sort of) distaste for global variables (and even singletons, which as just globals in disguise) has stopped my from thinking much of it. I can however be pragmatic. Besides, the world is effectively a global already, just expressed as a whole bunch of static variables scattered about. When done as a proper global, it is at least obvious that there is just a single instance active at any given time (in fact, there is just a single instance during the entire lifetime of the process from what I have found out, but that is not as important).

In any case, the variable itself should not be exposed, so as to prevent someone from accidentally writing to it. As for impact analysis, this shouldn't affect code that other patches normally touch, so the risk of merge conflicts is low. Patches might still stop compiling because they reference deleted variables or functions, or functions that no longer require the world as a parameter. But like all changes deep in the inner core of things, it affects a lot of things.

[neroden]

Which implementation would you think is best?

My main motivation here is that I don't want to have to pass "karte_t*" parameters to various methods I'm writing and various classes I'm editing.  So I don't really care which of my suggested implementations we use, but I'd like to use one of them.

Once we have written the "official global", we can strip the "local copies" out from one piece of code at a time; it doesn't have to be done all at once.

[Ters]

As simple as possible. I would just have a get_world() or world() function exposed through simworld.h, but I must say that I know little of this thing called const optimization.

There is however one advantage of all these world parameters (and member variables to a lesser extent) now that I think of it, even if there now and forever is only one world: Some of them are const, so you know that these places don't modify the world, just reads some information. A single global has to be non-const, which in turn means that it's free for all to modify the world (without trickery like casting away the const). These const karte_t pointer are however heavily outnumbered, either because so few places never modify the world, or because the developer was not sure about using const or simply lazy.

[prissi]

There ist spieler_t::get_welt() which is static if memory serves me right. There was even an unfinished(?) patch by truelight to remove all welt pointer altogether (since we only have one in the current implementation and probably forever).

The const welt entered relatively late, and hence is almost unused.

[jamespetts]

When I first saw that the world was an instance object, rather than its variables being static, I assumed that there must have been an intention to allow multiple worlds. If there is no intention to do this, then isn't the best option to have a static world? There could hardly be more appropriate a use of a "global" variable than to store the world.

[Max-Max]

If the world_t is a class (I'm only in the GUI stuff now days), then a singelton with a hidden constructor would be the best approach. Have a static member to create and/or return the instance.

In this way you can't create more than one world.

If the world can fit into a single class, you would be able to have parallel worlds

[Ters]

I don't like uninitialized instances, so I don't want a singleton. Singletons are overrated anyway in my opinion. Only simmain should initialize the instance.

[Max-Max]

Singletons are overrated anyway in my opinion

I don't follow you here, in what way are they overrated?

If simmain is the first one to call the singleton, simmain is the one to initialise it, every call after that will not initialise, only get the already initialised instance.

If you want to call it Init(), get_instance() or have them both doesn't matter. The point with a singelton is to prevent other instances to be created. As long you know how it works, sure it is pointless because you know how to use the global instance. But a singelton not only prevents mistakes, it also tells other developers, not as familiar with the code, that this object isn't intended to be created more than once.

[Ters]

If simmain is the first one to call the singleton

That's the thing. What if it isn't?

Singletons are fine for things like a logger, which can initialize itself and has no dependencies on anything but the OS and language runtime. I expect a singleton's get_instance() to just give me an instance ready to go and I shouldn't have to worry about it. Imposing some restriction that a particular part of the code must be the first to call get_instance() doesn't fit quite with the pattern as I understand it. While it is technically no better using for example a naked global pointer before it is initialized, uninitialized pointers is at least a concept I am familiar with. Maybe I have misunderstood the singleton pattern, a lot of people apparently do.

I don't propose the world being much different from a singleton. It's just the create-on-demand aspect I'm not at ease with. There should be one function for initializing the world, which it would be a fatal error to call twice, while get_instance() should also fail fatally until that method has been called.

It might be that karte_t as it is now is fully usable once the constructor has run, with no functions failing due to uninitialized member variables. If so, a singleton might work, but I'm not sure it can stay that way with the changes I thought up for my OpenGL backend without having ifs all over the place. Not that there has been any progress on that lately.

[Max-Max]

I really don't see the difference in calling an static init() from simmain compared to calling any other init function. Both methods can be abused in the same way.

If you only want a single class or function to be the only one that call the singelton's init routine, you can make the init() private and then the caller class/function as a friend. This will prevent a call from everyone not defined as a friend with the singelton.

Retriving a non initialised singelton instance still has the same vulnerability as a global pointer. If you want to mimic that behaviour, the singelton can just return NULL if it isn't initialised, or throw an exception.

The main difference here is that the singelton can do something if someone try to use in the wrong way, while a pointer is.. just a pointer...

Anyhow, your OpenGL project sounds exciting

[Max-Max]

While we are at it. Shouldn't the game settings settings_t be a global singelton as well? Or can we have more than one set of settings?

[prissi]

The code uses more than one setting during loading or reverting.

[Max-Max]

Alright, yes it makes sense. You can modify a copy and then press cancel in the dialogue

[Ters]

Retriving a non initialised singelton instance still has the same vulnerability as a global pointer. If you want to mimic that behaviour, the singelton can just return NULL if it isn't initialised, or throw an exception.

I just don't think it is really a singleton anymore if it can be NULL, but that's just nomenclature. It's more or less this that I propose.

Anyhow, your OpenGL project sounds exciting

It was, until I found out that what I was trying to do didn't work.

[neroden]

When I first saw that the world was an instance object, rather than its variables being static, I assumed that there must have been an intention to allow multiple worlds.

I believe the intention is subtler than that.  The intention is to allow for there to be *zero* worlds.  Either zero or one.

This, theoretically, allows for a clean "startup and shutdown" procedure when dumping one game and loading another.

This is a good reason for not making the world itself static -- we actually want the possibility that "world == NULL" during loading.  This is a distinct state.

---
Now, I can think of useful applications for "multiple worlds".  With a copy constructor, it could be used to implement loading and saving (in the background!).  It could be used to implement "test construction".  Et cetera.  I can't think of any applications for having two *active* worlds at a time, however; I can only think of applications for having extra worlds "trapped in amber". 

As a result, even if we did implement multiple worlds, most of the code would only want to look at the "current world", including *all* the GUI code.  So we would want a "current world" function anyway.  No mutator should ever want to look at code other than the "current world", though I suppose I can imagine where an accessor might.

Now, the only clean way to implement "multiple worlds" is for every single object in the world to carry around a pointer to the world.  Every planquadrat_t, every ding_t.  With 64-bit pointers, this seems like an extremely heavy memory overhead.  (Although GUI items, besch items, and other stuff which is determined strictly by the pak do not need these pointers.)

However, maybe it's worth it; we've certainly put some other heavyweight stuff into planquadrat_t.  What do people think?

---
Anyway, back to the topic.  How about the following plan:
* static private variable in karte_t, "karte_t* current_world", which contains the *current world* or NULL if there is no current world.

* global function, "const karte_t* world()", (friend of karte_t) which returns it. 

I don't want to use get_world() and I don't want to use karte_t::anything, because these are too long for lazy programmers to type.  The world shows up all over the place and people want its name to be SHORT.  I even found one function which saved a copy of the pointer under the name "w".  Perhaps the function name should actually be w() ?  But perhaps that is too short.  I am open to suggestions regarding the function name.

* simmain.cc will be allowed to reset the value of current_world.   So, for instance this would be permitted in simmain.cc:

Code Select Expand


current_world.save(backup_file);
delete current_world;
current_world = NULL;
current_world = new karte_t(...etc...);


Does this sound like a good plan?

(In the future, if we ever do decide to have "worlds frozen in amber", this would be permitted:

Code Select Expand


karte_t* backup_world = world;
world = NULL;
world = new karte_t(...etc...);

... or even this:

Code Select Expand


karte_t* backup_world = world->make_copy();

but mutator code would still only be permitted to work on the current world.)

[Ters]

neroden's suggestion sounds good to me.

[prissi]

A copy of the world, i.e. a copy of every object in it will most likely rather increase the saving and loading. Just look at how long simply the destruction of a world take.

[neroden]

OK, this is my next project, but I'm not going to start it until the "stacked stations" patch is integrated. 

This thread should probably be moved to "larger projects".  I'm not going to try to do this as a single patch, it will be a series of patches.

[prissi]

If access to welt, I would be for karte_t::current or karte_t::w. That is almost the same as current_world, but as short and more clear where is comes from.

Even if you have more than two worlds, you can make a thread for loading which returns the old world for freeing up stuff and a new pointer for creating&loading the new one. Then world might be thread local.

[neroden]

If access to welt, I would be for karte_t::current or karte_t::w. That is almost the same as current_world, but as short and more clear where is comes from.

Given that programmers in some places have already used a local variable "w" just to type less, I think karte_t::w() is too long!  Also we will eventually want to change the name of karte_t to something in English and I'd rather reduce the number of places it has to be changed.

So I would really rather use "world()".  I think this is a short as I can make it while still being clear about what it is.  It is the current world, the functions using it don't care where it comes from...

Perhaps I will write karte_t::get_current_world() and have "world()" simply call that, as a "convenience function"?

Even if you have more than two worlds, you can make a thread for loading which returns the old world for freeing up stuff and a new pointer for creating&loading the new one. Then world might be thread local.

Yes, this would make sense.  So it is OK for each thread to treat "world" as if it is a single variable.

[Dwachs]

I implemented something along Neroden's suggestions. See attached patch. The idea is to define a wrapper class to encapsulate all the static pointers to the welt-pointer.
The wrapper class contains the pointer to the world. The wrapper class registers itself on construction to a global list. If the main world is instantiated the wrapper class member will be set to the new world pointer. I added convenience operators to enable the use of the wrapper class as if it were a karte_t* pointer.

See the minimalistic changes in simfab.*: the static variable is changed from

Code Select Expand


static karte_t *welt

to

Code Select Expand


static karte_ptr_t welt

The karte_ptr_t class will take care about initialization, dereferencing (welt->bla), and casting to karte_t*.

This patch actually also fixes a bug described here:
http://simutrans-forum.de/forum/index.php?page=Thread&threadID=8135
There is a corner-case where the static pointer is NOT initialized at all before first use.

[Markohs]

 Why not just making it a singleton? Updating the references on karte_t creation it's a very clever idea, but it's just "fixing" a problem that shoudn't had happened in the first place imho. I'd personally implement it as a singleton, and make sure it's the first thing that's created in simu_main, in line of the singleton idea code it's on my framework patch "singleton.h".

Considering welt_t class is instantiated just once, I think it's the best option.

Anyway it's a nice implementation, I'm not against this hitting trunk.

[Ters]

Why not just making it a singleton? Updating the references on karte_t creation it's a very clever idea, but it's just "fixing" a problem that shoudn't had happened in the first place imho. I'd personally implement it as a singleton, and make sure it's the first thing that's created in simu_main, in line of the singleton idea code it's on my framework patch "singleton.h".

Considering welt_t class is instantiated just once, I think it's the best option.

Seems simpler, though it's not what I would call a singleton.

[Markohs]

Seems simpler, though it's not what I would call a singleton.

Why?

singleton.h

(I notice now I'm using (void) argument declaration there, that should be fixed too )

Code Select Expand


/*
* Copyright (c) 2013 The Simutrans Community
*
* This file is part of the Simutrans project under the artistic license.
* (see license.txt)
*/

/*
* Singleton
*/

#ifndef singleton_h
#define singleton_h

#include <assert.h>

template <typename T> class singleton
{
private:
    singleton(const singleton<T> &);
    singleton& operator=(const singleton<T> &);
   
protected:

    static T* static_singleton;

public:
    singleton( void )
    {
        assert( !static_singleton );
        static_singleton = static_cast< T* >( this );
    }

    ~singleton( void )
    {
        assert( static_singleton );
        static_singleton = 0;
    }

    static T& get_singleton( void )
    {
        assert( static_singleton );
        return ( *static_singleton );
    }

    static T* get_singleton_ptr( void )
    {
        return static_singleton;
    }
};

#endif


[Dwachs]

Why not just making it a singleton?

I hesitate to do this, because then the karte_t is instantiated before anything else is done, in particular all the settings are not read from the config files yet, no pak-set is loaded etc, which might or might not have bad consequences. The approach I implemented seemed to be more robust against such type of programming errors.

[Ters]

This is a singleton:

Code Select Expand


class Singleton {
private:
    static Singleton instance;

    Singleton();
    ~Singleton();

public:
    static Singleton *getInstance() {
        return &instance;
    }
};

as is this (although in C++ it won't get it's destructor called):

Code Select Expand


class Singleton {
private:
    static Singleton *instance == null;

    Singleton();

public:
    static Singleton *getInstance() {
        if (!instance) {
            instance = new Singleton();
        }
        return instance;
    }
};

There are also other variations, but a common theme is that the constructor is private. Another thing is that the rest of the system doesn't know when the instance is constructed/initialized, only that it has happen by the time getInstance() returns.

[Markohs]

I hesitate to do this, because then the karte_t is instantiated before anything else is done, in particular all the settings are not read from the config files yet, no pak-set is loaded etc, which might or might not have bad consequences. The approach I implemented seemed to be more robust against such type of programming errors.

That's true, but not forcing it to be instantiated as the program starts has the problem you might end calling a NULL pointer. It's also a solution specifically designed for karte_t,  not generic.

Anyway, as I said before I like your solution,  nothing against it.

@Ters: I see. The potential problem I can see there is you end not knowing exactly when it will be instantiated,  if refactoring you do a get_instance  before it was, you alter instantiation time, that might have side effects.  It's also sightly slower because of the test.

[Dwachs]

That's true, but not forcing it to be instantiated as the program starts has the problem you might end calling a NULL pointer. It's also a solution specifically designed for karte_t,  not generic.

Of course. But for karte_t it should not be a problem: the world is created around line 1073 in simmain.cc. Before this only settings and pakset are read. I also did not want to add checks whether the pointer is already initialized.

[Ters]

@Ters: I see. The potential problem I can see there is you end not knowing exactly when it will be instantiated,  if refactoring you do a get_instance  before it was, you alter instantiation time, that might have side effects.  It's also sightly slower because of the test.

True. Your code however has the problem that if you refactor and end up with get_instance before someone has initialized it, it has a very certain side effect of crashing. I'd also like to replace your constructor and destructor with static functons called something like initialize and shutdown, because it makes it clearer, without looking at the implementation, that these are one-shot functions.

[Markohs]

I also did not want to add checks whether the pointer is already initialized.

That sounds like a good reason to me. Ok.

True. Your code however has the problem that if you refactor and end up with get_instance before someone has initialized it, it has a very certain side effect of crashing. I'd also like to replace your constructor and destructor with static functons called something like initialize and shutdown, because it makes it clearer, without looking at the implementation, that these are one-shot functions.

I also was thinking similar to that, but for a different reason. The problem in doing all initializarion in the constructor is that you can't return an error code when something fails. In the systemmanager for example, I can fail if I can't open the window. Adding that function I'd be able to do so, even I'd sugegst adding a default contructor parameter (bool auto_init = false) so the singleton is automatically initializated (with its error ignored), or let the user the decision of initializing it manually, after the instantiation.

If you want to implement it yourself, I'd be happy to accept it.

[isidoro]

[...]
as is this (although in C++ it won't get it's destructor called):

Code Select Expand


class Singleton {
private:
    static Singleton *instance == null;

    Singleton();

public:
    static Singleton *getInstance() {
        if (!instance) {
            instance = new Singleton();
        }
        return instance;
    }
};

[...]

Another problem with this code, apart from the minor bug in

Code Select Expand


    static Singleton *instance == null;

is that it is not thread safe...

[Markohs]

just one extra question Dwachs, will your implementation still allow for forward declaration in the .h files of the karte_t class?

Like this:

Code Select Expand


#include "blablabla"
// (No #include "simworld.h", just in the .cc file)

class karte_t;

class example {
private:
   karte_t *world;
}


[Ters]

Another problem with this code, apart from the minor bug in

Code Select Expand


    static Singleton *instance == null;

is that it is not thread safe...

Both are effects of coding in a forum edit box. The two code snippets are not meant to be working examples, just something that gives an impression of the basic idea.

[Dwachs]

just one extra question Dwachs, will your implementation still allow for forward declaration in the .h files of the karte_t class?

This is a valid question. I have to test. But I would expect that karte_ptr_r needs to be available and not only forward-declared. Maybe this wrapper class has to go into a separate header then.

[prissi]

Sorry, I am a little at loss of the many stuff here.

So why npot make  a static member of karte_t called w. karte_t::w will be either NULL (bei definition in the header file) or will be set in init (the first point karte_t contains useful content) to this. Simworld.h is anyway include everywhere where the karte_t this is needed.

Code Select Expand


in simworld.h

#define WORLD (karte_t::w)

class karte_t
{
static karte_t *w;
...
};

in simworld.cc

karte_t karte_t::w = NULL;
....

void karte_t::init()
{
...
karte_t::w = this;
}

void karte_t::destroy()
{
....
karte_t::w = NULL;
}

everywhere else
welt -> karte_t::w
or
WORLD->

Using a define would allow for a different funtion for debug levels>3 ...

This is threadsafe by desgin, would also allow for testing karte_t::w for NULL (map may exist but not properly no initialized or is already destroyed), and you need to include no new header. Accessing a static is a fast as it could be. I fail to see the advantage of a special pointer class, when there should be no test on accessing it anyway. But I admid I may be thick, and happily hear more explaination.

[Dwachs]

The idea was to implement something that needs as less modification of other files as possible. This means that the static members 'karte_t* welt' in the code still are static members (now of a different type).

Macros may or may not lead to different problems if 'welt' is replaced by 'karte_t::w' and the word 'welt' is used in a different context, for instance as name of a function parameter. I will try.

[Markohs]

I'd stay away from macros, they are allways a source of problems.

[prissi]

Ok, I can understand to stay away from macros.

karte_prt_t could achieve the same if it returns karte_t::welt and still karte_t  decides when it is time for initialisation.

Code Select Expand


class karte_ptr_t
{
karte_t* operator->() { return karte_t::world; }
...
}

and init then in karte_t::init() and NULL in destroy.

The extra list of all instances is not needed, respective only needed if there ever going to be more than one world (even then a static access would work). Also the list seems not very thread save. Or am I overlooking something again?

EDIT: forgot the patch

[Dwachs]

this is now incorporated, r685x.

[Dwachs]

Here is a follow-up patch that removes the 'karte_t*' parameters in functions of fabrik_t, some related gui windows lost karte_t* parameters as well. Should we proceed in this direction? Ie transform the code to not pass any karte_t parameters around?

[kierongreen]

Well if there's no need to then seems a bit pointless passing them around.

[prissi]

I agree not passing around. However, MSVC complains that struct cannot be freind, class can. So I declare karte_ptr_t a class then ...

[Dwachs]

The big patch is committed r6871