DarkRadiant and VC++ 2005

[greebo]

I managed to assemble a VC++ 2005 project with the current DarkRadiant source. This was quite an adventure, I had to define all the plugins and dependencies anew, the old GtkRadiant project is already so far away that it was only of minor use. Additionally, I had to build all the boost and OpenAL static libraries from scratch.

 

There were a lot of things complained about by the VC compiler, some of them were quite strange, but there were also quite a few occurrences of cross-includes (e.g. plugin/mapdoom3 includes radiant/brush/TexDef.h - it doesn't need it, obviously a leftover, but the scons environment apprently declares radiant/ as default include directory).

 

Also, a few private/public issues were reported and a ton of warnings about loss of data by doing type conversions. The old C libraries ddslib and such also threw a few warnings, which I could fix by commenting a few lines out.

 

Another point was the inclusion of , which includes a __gxx_map hashmap I couldn't figure out - it only exists in GCC, so I didn't stand a chance with MSVC here. I just replaced it with a std::map, but I can't say if this breaks the string>GtkIterator* mapping or anything.

 

However, I'm able to use the VC++ debugger with DarkRadiant under Win32 now, which is an advantage for me (I can't get gdb to work without random crashes). I don't intend to use it as a primary build environment, as I'm quite fond of the Eclipse IDE, but VC++ has its advantages (debugger and compile speed - it's fast, MinGW/scons is a snail in comparison).

 

Anyway, my question is: Can I create another branch for the VC++-compatible code? I don't want to commit the VC-related fixes to the trunk for sure, but I don't want to lose my code changes and all the project/solution files either. Is this ok?

 

Another question: Do we want to keep the door open for other compilers or is it enough for us to run under GCC?

[OrbWeaver]

Another point was the inclusion of <ext/hash_map>, which includes a __gxx_map hashmap I couldn't figure out - it only exists in GCC, so I didn't stand a chance with MSVC here. I just replaced it with a std::map, but I can't say if this breaks the string>GtkIterator* mapping or anything.

 

That's fine, we should not be using the __gxx extensions anyway since they are non-standard. I doubt using an ordinary std::map is a performance issue anyway, so feel free to replace all of these for good.

 

Anyway, my question is: Can I create another branch for the VC++-compatible code? I don't want to commit the VC-related fixes to the trunk for sure, but I don't want to lose my code changes and all the project/solution files either. Is this ok?

 

I don't mind having VC++ files in the main trunk, since they can just be ignored by those who don't need them. Obviously if I make changes under Linux I won't be able to update the VC project, but this is standard procedure in the main mod anyway.

 

If the code fixes you have made for VC++ don't break standardisation and are good design then they can also go into the main trunk, if they are ugly Microsoft-specific workarounds then they would indeed need to go into a separate branch.

 

Another question: Do we want to keep the door open for other compilers or is it enough for us to run under GCC?

 

We should aim for 100% standardised C++, which should be compilable under any toolchain. Generally GCC implements standards better than Microsoft, so there is unfortunately a chance that standard code might not work under VC++; in this case the odd #ifdef _MSC_VER might be needed, but hopefully there won't be too many of these.

[greebo]

Heh, I ran into my first issue. In treemodel.cpp I found this gem here:

iter->stamp = c_stamp; // iter is of type GtkTreeIter*
*reinterpret_cast<GraphTreeNode::iterator*>(&iter->user_data) = i;

Which should store an STL map iterator in the user_data fields of an GtkTreeIter (!). Curiously, this seems to work when using GCC, but incidentally, the STL iterator size differs from toolset to toolset (in Visual C++ 2005 the size of the iterator is 12 bytes instead of 4 bytes in the GNU implementation.

 

Thank you, GtkRadiant - this is the perfect excuse to redesign this graph tree model crap, which is a bunchload of C-code anyway.

[OrbWeaver]

Surely it is storing a pointer to an iterator, which should be 4 bytes on a 32-bit system?

 

It looks like the scene graph is supposed to implement the GtkTreeModel interface or something, which is a pretty daft way to design something which has nothing to do with representing GUI tree views.

[greebo]

Surely it is storing a pointer to an iterator, which should be 4 bytes on a 32-bit system?

*reinterpret_cast<:iterator>(&iter->user_data) = i;

 

I can't be entirely sure here, but I'm confident that this is not a pointer storage. First, the adress of the GtkTreeIter.user_data field is taken and hard-cast onto a GraphTreeNode::iterator*. This is subsequently dereferenced and the contents of the function argument GraphTreeNode::iterator i are assigned to it.

 

It looks like the scene graph is supposed to implement the GtkTreeModel interface or something, which is a pretty daft way to design something which has nothing to do with representing GUI tree views.

The treemodel is supposed to be a "mirror" of the actual scene nodes, the EntityList is relying on that model. The cool thing is that there are some callbacks hacked into the actual scenegraph just to accomodate for the selection status of the EntityList. It's an expensive callback chain whose purpose is to determine whether the parent of an instance is selected. Each instance is crawling up its ancestry to check that. Multiply that times the number of scene nodes and you get one of the reasons why the scenegraph selection changes are so slow.

[OrbWeaver]

*reinterpret_cast<GraphTreeNode::iterator*>(&iter->user_data) = i;

I can't be entirely sure here, but I'm confident that this is not a pointer storage. First, the adress of the GtkTreeIter.user_data field is taken and hard-cast onto a GraphTreeNode::iterator*. This is subsequently dereferenced and the contents of the function argument GraphTreeNode::iterator i are assigned to it.

 

OK, so the iterator is being copied to the memory location whose address is stored in the user_data field. Provided that this memory address still points to valid memory, and that GraphTreeNode::iterator has a functional copy constructor this should not be a problem in itself (from a code point of view at least, I'll bet that the design is crap).

 

The treemodel is supposed to be a "mirror" of the actual scene nodes, the EntityList is relying on that model. The cool thing is that there are some callbacks hacked into the actual scenegraph just to accomodate for the selection status of the EntityList. It's an expensive callback chain whose purpose is to determine whether the parent of an instance is selected. Each instance is crawling up its ancestry to check that. Multiply that times the number of scene nodes and you get one of the reasons why the scenegraph selection changes are so slow.

 

Usual convoluted rubbish, then (in English "cool" usually means something good, not bad). I guess with the new module system it should be relatively easy to separate the scene graph into its own DLL (it is already a module obviously, but I never managed to get it to work in its own DLL because of all those stupid initialisation errors), which would make it easier to replace with a new design.

[greebo]

OK, so the iterator is being copied to the memory location whose address is stored in the user_data field. Provided that this memory address still points to valid memory, and that GraphTreeNode::iterator has a functional copy constructor this should not be a problem in itself (from a code point of view at least, I'll bet that the design is crap).

No, the iterator is copied over the actual memory of the void* user_data itself. It's overwriting the pointer's very own memory.

 

Storing 12 bytes in the GtkTreeIter could work, because the GtkTreeIter provides three pointers (user_data, user_data2 and user_data3), but the VC implementation seems to have stricter memory checks, as trying to execute the above expression throws an exception.

 

(in English "cool" usually means something good, not bad).

I was being sarcastic.

Usual convoluted rubbish, then

Fully agreed.

[OrbWeaver]

No, the iterator is copied over the actual memory of the void* user_data itself. It's overwriting the pointer's very own memory.

 

Ah yes, I didn't notice the & before iter->user_data. Yes, this is abominably retarded and stupidly assumes that an std::map::iterator is going to be 4 bytes in size.

 

It's strange that there would be such a fundamental error though, because GtkRadiant used to be compiled with VC++ under Windows.

[greebo]

Ah, I assume that's why there is an STLPort package is shipped with the GtkRadiant dependencies.

[OrbWeaver]

That may well be the case; it is quite plausible that VC++ did not have good STL support back when GtkRadiant started.

[greebo]

I merged in all the changes required to get it to compile and link under VC++. All the project files and the main solution file is under tools/vcprojects.

 

Of course DarkRadiant crashes at startup due to the iterator issue above, so this will have to wait until the scenegraph refactor.

 

This will probably need its own thread, but I guess we will have to define a SceneGraph::Observer class which gets notified about instance insertions and deletions as well as a SelectionSystem::Observer, which gets notified about selection changes. The EntityList can derive from both observer classes, which should be enough to display the graph of selectable instances.

[OrbWeaver]

Yes, that sounds reasonable.

 

I've only got as far as the business requirements for the scenegraph (no design yet), but I'll post them anyway since they might help to guide the design ideas.