Hi.

I've asked this on a couple of other forums and got some useful responses but I'm still not 100% sure as to the best way to proceed with this. I've got this problem of representing a "weird" kind of graph structure, which is like a normal graph but it has an additional layer of structure called "strings" which are composed of nodes connected together and ordered in a linear order. The purpose of this graph is to represent a system of levels in a game. In one of the responses I got, it was suggested I should implement this using a conventional graph type (i.e. just an implementation of a conventional graph, like Boost's graphs). But the problems appear when considering the "strings". Namely, I have something like this:

class WeirdGraph {
private:
class NodeString {
private:
<...>
std::vector<WeirdGraphNodeIterator> nodes;
<...>
public:
<...>
};

<... (there is a graph variable declared here) ...>
std::vector<NodeString *> strings;
<...>
public:
<...>

};

i.e. the weird graph object contains a private class to represent the strings. Problems are that: 1. iterators in the graph get invalidated. Each node string stores the nodes comprising it as iterators into the graph. When a node is deleted, the iterators get invalidated, and this seems to require an update to the NodeStrings. What can be done? Obviously, one could just use a graph that does not invalidate iterators (though this may lead to a penalty accessing/traversing the graph, hence why I'm a little hesitant, since I'm going to be accessing more than I'm going to be deleting stuff, though I doubt this is used in performance-critical code in any case so it probably doesn't matter.). But I'm curious to know if there may be a technique that would work even in the case of invalidating iterators. 2. To update, the node strings need to know what nodes belong to them. So the nodes contain a string ID field, which is user-specified when the user creates a string and the WeirdGraph enforces uniqueness. Is it "good form in C++" to have WeirdGraph do the enforcing and not NodeString? It feels iffy. 3. Regarding that, is there an alternative to using the user-specified string IDs that won't fail (e.g. incrementing a counter may fail long off into the future, but I doubt one will be creating and deleting billions of times. But is having that "latent failure" possibility in there OK? As it feels iffy too.)? 4. NodeString constructs nodes in its constructor, then deletes them in the destructor. This last bit makes me feel iffy as then it's not okay to pass one by value -- though we can always not do that, but I'm not sure whether having such requirements which are enforced only by user understanding is a good idea (although does it matter here since NodeString is a private internal of WeirdGraph?).

Any comments on these issues? Right now I'm really leaning toward using a non-invalidating graph type, as that would entirely eliminate 3 out of the 4 issues above, but I'd still like to know what the deal is on the 4th. The 4th is also why I'm using pointers above (perhaps I should use std::auto_ptr instead of raw pointers there? hmm) instead of objects directly, since that would necessitate some copying/by-value passing when filling that vector.

• You shouldn't delete things through iterators; you need iterators to store pointers or IDs of the items you need to delete, but after you have finished the search you don't need the iterators any more.
  
• Can't you store node IDs in the NodeString instead of NodeString ids in the node? How do you represent the order of nodes in NodeString?
  
• C++ is full of objects that should not be copied for various reasons. If you are afraid of making a mistake, you can either enforce noncopyability with something like boost::noncopyable or make your class safely copyable; in your case you could define a copy constructor that doesn't cause trouble, probably using smart pointers to avoid deleting the same memory twice.
  

1. The iterators are so the string knows what its nodes are and can easily access them. And the graph type needs to be passed an invalidatable iterator (in the case of my home made graph class) or "descriptor" (in the case of Boost's graph which I've thought of switching to) to tell it what node to delete.

2. Store node IDs, not iterators? Then we'd need a costly search through the graph to look up the corresponding node's ID just to access it. Iterators provide O(1) access. As for order, there's a vector of node iterators in NodeString. (Should go and update the OP to show that. Just did it.) Then the order of nodes is simply the order in which those iterators come. The nodes are also numbered internally, and the graph has an order to its nodes as one iterates over them, that is like a vector.

3. Thanks for the answer there, but "safe copying" looks to be out of the question, considering the destructor deletes the nodes from the graph, that has nothing to do with "smart pointers", but with the graph. Where else should I delete the nodes of a destroyed string?

You should:

• Explain the operations of your abstract data type more precisely, which is crucial to finding a solution that covers all needs.
  
• Post complete code samples. Details can be important, but in this case you are omitting non-details too: [font=arial,helvetica,sans-serif]the WeirdGraphNodeIterator[/font] type, where are Node objects stored, what exactly is allocated in constructors and deallocated in destructors to make your classes unsafe to copy, and many other large gaps between guessing what you are doing and having something to give advice about.
  

1. The operations are:
add string,
remove string,
connect two strings (needs to have level numbers of the levels in the strings that are connected specified and what this does is form an edge in the
graph joining the two strings at the indicated nodes (indicated by the level numbers)),
fetch a map from a level in a string,
get/set various web attributes and string attributes (latter not yet implemented).
Strings should be referenceable by both array index (e.g. like a "vector" of strings would be) and by their special ID code (which does not change
when deleted and is the preferred method for referencing them elsewhere).

2. I've got about ~800 lines in the current implementation, and that's not counting the graph class. Do you want me to post all that code here?
But I can tell what makes strings unsafe to copy: The constructor is passed the graph on which to construct the string, by reference. It then constructs the nodes
corresponding to the given string (via, of course, the graph class's interface) in there. The destructor does the opposite, deletes all referenced nodes. It is not safe to
copy, since that would cause additional nodes to be created, with the same ID as the previous nodes, and once that copy destructor is invoked it would destroy both sets
of nodes due to them having the same IDs. Now, changing IDs on copy would mitigate the last concern, but you're still altering the graph structure on copy, and that
might not be what you want when passing those strings to other functions (so it seems by-reference argument passing is _mandatory_).
"WeirdGraphNodeIterator" is just an instance of an iterator in template that the general graph class uses. That general class gives objects of the form
Graph where V is the type of object to store in each node and E is the type to store in each edge, e.g. Graph would store an int on
each node and edge. In this program, WeirdGraphNodeIterator is Graph<LevelElement, LevelConnectionData>::NodeIterator, where the two
mentioned items store the data that defines respectively the levels and the properties of connections between them, and the NodeIterator is just the
graph's node iterator type.

If I understand your description correctly, you have a standard graph, and then you have a set of 'strings', each of which is an ordered subset of nodes in the graph. Is that correct?

Now, you mention deleting nodes from the graph. Under exactly what constraints can deletion occur: can you delete a node that is referenced by one or more strings? Does that deletion also cause the relevant strings to be deleted?

Yes, you are right in your basic understanding.

No, you can only delete whole strings right now, though having individual-node deletion capability may be useful.

Though I'm hesitant to post the full 800-line behemoth, I can post the class definition for the current implementation. Here it is:

(and OH CRAP, the forums seem to have bungled the formatting! )

(Note here that some terminology is different since this is a concrete realization. The "weird graph" is obviously not really called that,
so here it's called "LevelWeb", and the "strings" are called "LevelBranch"es.)

class LevelWeb {
private:
/* Level element. This is used to form the nodes of the level web's graph. */
class LevelElement {
private:
int branchID; /* ID code of branch this level belongs
* to.
*/
int levelNumber; /* Level number of this level in its
* associated branch.
*/
LevelGenerator *levelGenerator; /* Level generator used to generate
* this level.
*/
bool isGenerated; /* Flag indicating whether this level
* has been generated.
*/
std::string fileName; /* Filename of file to store map in */
public:
LevelElement(); /* Default constructor */
LevelElement(int, int, LevelGenerator *,
std::string); /* Construct to specified parameters */
~LevelElement(); /* Destructor */
int getBranchID(); /* Get this level's branch ID code */
int getLevelNumber(); /* Get this level's level number */
void setLevelNumber(int); /* Set this level's level number */
bool isLevelGenerated(); /* Returns whether this level has been
* generated.
*/
LevelMap *retrieveMap(); /* Retrieve the level map. Generates
* the map if it's not already generated
* or there's no map stored.
*/
XXXXXError saveMap(LevelMap &); /* Save a level map to the file */
};

/* Level connection data. This is stored in the graph's edges. */
struct LevelConnectionData {
PortalType portalType; /* Type of portal used for this
* connection.
*/

/* Constructors */
LevelConnectionData() : portalType(PORTAL_UPSTAIRS)
{}
LevelConnectionData(PortalType portalType_) : portalType(portalType_)
{}
};

/* Graph data types */
typedef Graph<LevelElement, LevelConnectionData> WebGraph;
typedef Graph<LevelElement, LevelConnectionData>::NodeIterator WebGraphNodeIterator;
typedef Graph<LevelElement, LevelConnectionData>::EdgeIterator WebGraphEdgeIterator;

/* Level branch. The web is composed of branches, linked together in
* the underlying graph.
*/
class LevelBranch {
private:
int parentWebID; /* Parent web ID code */
int branchID; /* Branch ID code. Must be unique
* for each branch in the web.
* Used to identify the branch
* _and_ to tag the nodes in the
* graph.
*/
std::string branchAbbrev; /* Abbreviated description of this
* branch.
*/
std::string branchDescriptionText; /* Text description of this branch. */
WebGraph *parentGraph; /* Graph containing the nodes comprising
* the branch.
*/
std::vector<WebGraphNodeIterator> nodes; /* Graph nodes containing the
* levels in the branch.
*/
public:
LevelBranch(); /* Default constructor */
LevelBranch(int, int,
std::string, std::string,
WebGraph *,
LevelGenerator *,
PortalType,
PortalType,
std::size_t); /* Construct to specified parameters */
~LevelBranch(); /* Destructor */
int getBranchID(); /* Get branch ID */
std::size_t getNumLevels(); /* Get number of levels in this branch */
std::string getAbbrev(); /* Get abbreviated description of this
* branch
*/
std::string getDescriptionText(); /* Get branch description text */
bool isLevelGenerated(std::size_t); /* Returns whether a given level has
* been generated.
*/
LevelMap *retrieveMap(std::size_t); /* Retrieve a level map from the
* branch
*/
XXXXXError connectTo(LevelBranch &,
PortalType,
std::size_t,
std::size_t); /* Connect this branch one-way to
* another branch.
*/
void recalibrate(); /* Recalibrate this branch to the
* parent graph. Necessary after
* removing nodes from the parent
* graph (or any other operation that
* invalidates graph iterators).
*/
};

int webID; /* Web ID code */
std::string webAbbrev; /* Abbreviated description of this web */
std::string webDescriptionText; /* Text description of this web. */
WebGraph graph; /* The underlying graph containing all
* the level data.
*/
std::vector<LevelBranch *> branches; /* The level branches, which are defined
* on the graph.
*/

/* Routines defined in levelmgmt.cpp */
std::size_t searchForBranch(int); /* Private member: searches for a
* level branch with a given ID code
* and returns its index (or -1 if
* no branch found)
*/
public:
/* Routines defined in levelmgmt.cpp */
LevelWeb(); /* Default constructor */
LevelWeb(int,
std::string, std::string);/* Construct with specified parameters */
~LevelWeb(); /* Destructor */
void addBranch(int,
std::string, std::string,
LevelGenerator *,
PortalType,
PortalType,
std::size_t); /* Add a new level branch to this web. */
XXXXXError removeBranch(std::size_t); /* Remove a level branch from the
* web.
*/
XXXXXError removeBranchByID(int); /* Remove a level branch indicated by
* its ID code.
*/
XXXXXError connectBranches(std::size_t, std::size_t,
std::size_t, std::size_t,
PortalType, PortalType); /* Connect two level
* branches
* together.
*/
XXXXXError connectBranchesByID(int, int,
std::size_t, std::size_t,
PortalType, PortalType); /* Connect two level
* branches,
* indicated by
* their ID codes.
*/
LevelMap *retrieveMap(std::size_t, std::size_t); /* Retrieve a level map. */
LevelMap *retrieveMapByID(int, std::size_t); /* Retrieve a level map
* with branch indicated by
* its ID code.
*/
};


Also, note that in the above code that routines not marked "byID" take indices which index into the vector of branches. Adding and removing branches is then like adding and removing stuff to/from a vector.

Though I'm hesitant to post the full 800-line behemoth, I can post the class definition for the current implementation. Here it is:

(and OH CRAP, the forums seem to have bungled the formatting! )

(Note here that some terminology is different since this is a concrete realization. The "weird graph" is obviously not really called that,
so here it's called "LevelWeb", and the "strings" are called "LevelBranch"es.)

My first thought is that storing iterators (or pointers) is "evil" in the case of an editable structure such as this. I would (assuming it is possible for you) start by separating an editable version from a final version. This is not solving your problem directly but instead solving via alternative thinking. I always setup my build environment to have a "retail" build which defines XXX_BUILD_RETAIL as a 1 or 0 depending on the build. In the case of this code, I would separate everything into map/multi_map's of data each with unique ID's as the "editable" (well, I usually call it "Mutable"). Then, storing tuples referencing the id's instead of iterators. (Tuple is a vector of id's. Reference a level with "level id", a level element with "level id, element id", etc.)

Ok, that slows things down, makes it all larger and sucks if you used the separated id mapped data directly within the game. But that is not what you do at all. Any "edit" to the mutable structure calls a "Save" function which blows away the engines current world graph and rewrites a completely new version all optimized for engine usage which includes pointers/iterators or whatever is best for the "real" data. All the "mutable" code compiles out in retail and you end up just loading the optimized items directly instead of using the mutable version to control the loading. The general code description is the following:


class LevelMap
{
... the retail build required functionality and data ...
basically your current code removing all non-const functionality.
};

#if !BUILD_RETAIL
class MutableLevelMap : public LevelMap
{
editing functions and complete duplication of all data in a non-optimized manner.
the "save" function rewrites the LevelMap data.
};

typedef MutableLevelMap LevelMap_t;
#else
typedef LevelMap LevelMap_t;
#endif


That is just one way to solve the problem of mutable during dev versus const at release. I can think of a lot of other variations but this is my old stand by when possible. Of course if the save takes too long you can always look into proper selective updating of the final data. If memory is a problem, get a bigger machine since this is only for development purposes. (Really, get a bigger machine! )

Sorry if this isn't usable for you, I just like posting the "right angle view of the problem" first as it can often break loose other ideas to attack something this complicated in terms of dependencies. I can see a couple ways to reorganize the data which could help but my suggestion for that direction goes into another of my favorite 90 degree view points. Ask at your own risk.