wmarkham
Level: Master Delver

Rank Points: 125
Registered: 12-06-2004
IP: Logged

I recently mentioned in the "open source?" thread ( http://www.drod.net/forum/viewtopic.php?TopicID=4499 ) that I have been playing around with the DROD source code. I have made various modifications to my own private copy. Almost all of these are stylistic in nature, which is to say that they do not change the observable behavior of the game in any way. I have been working with Linux and g++, which is already a supported architecture out-of-the-box. Therefore, my efforts are not likely to generate any new ports. However, a few of the changes that I've made could potentially be useful to people who are trying to port the DROD code to new architectures.

So, I'm starting this thread for a couple of purposes:

1. Present a high-level list of changes that I have made, and/or that I think could be generally useful. If someone were to say "hey that sounds useful to me", then I would probably be willing (time permitting) to generate diffs against the code in CVS that effect the described change.

2. Provide a forum for discussion of some particulars of the DROD source code.

If other people have similar comments and/or questions about the code, they might consider posting within this thread. (It looks to me like most of the other threads in this forum are mainly concerned with data-only mods to DROD. Is that accurate?)

More to follow shortly...

wmarkham
Level: Master Delver

Rank Points: 125
Registered: 12-06-2004
IP: Logged

Here are stylistic changes that might ease the efforts needed in order to port the code to a new architecture:

- I use "#ifdef WHM_HAVE_FMOD" around any FMOD-specific calls. This replaces "#ifdef __sgi" that is already there. The benefit is that it is not specific to any platform, so that different ports can include/exclude the functionality very simply, without additional modifications within the code that calls FMOD.

- In the future, I may eliminate the dependency of my Sound.h header file on <fmod.h>. This has the minor benefit that different implementations of Sound.cpp become link-compatible. In other words, one could choose the with-fmod or the without-fmod version at link time. To take this one step further, one could define virtual functions in CSound for playing songs and effects, then provide different implementations for different underlying sound libraries. The single instance that is constructed and used could be selected at runtime.

- I use "#ifdef _MSC_VER" instead of "#ifdef WIN32" to detect platform differences that are related to the compiler or the runtime library. This eases porting to other compilers for Windows. As long as SDL supports it, it should be easy to compile a Windows executable with the g++ compiler. (using Cygwin or mingw libraries; I believe I compiled all the .cpp files like this at some point, but didn't bother to link it, due to the external dependencies)

- using enumerated types, inline functions, and function templates can eliminate many of the uses of preprocessor macros that appear within DROD headers, which can potentially conflict with names used in library header files. Admittedly, system headers should not be sensitive to most of the macros that are defined, and other libraries' headers will be similar across other platforms. I have been replacing these as I encounter them.

- use of namespaces would further prevent name conflicts with libraries. I haven't bothered to look into the changes needed, because they wouldn't have much benefit to me. I just thought I'd mention it for the sake of completeness, since it ties in with the above point.

- rewrite of DbPackedVars. I eliminated the return values from the SetVar member functions. I don't think that they are ever used. Casting a "void*" that refers to arbitrarily-aligned memory into a pointer to a specific type is undefined (non-portable) behavior. (except for char and unsigned char types) Internally, I simply copy raw memory into the storage for a return value, which is returned by value. Byte swapping might need to be performed here as well. (See a question I ask later...)

- I have been replacing the conditionally-compiled ("#ifdef __sgi") calls to "LittleToBig" functions with unconditionally-compiled "externalToInternal" and "internalToExternal" functions. On a little-endian machine, these do nothing; on a big-endian machine, they call the appropriate "LittleToBig" functions. The main benefit to porting efforts is that additional ports can simply provide the appropriate definition (by ensuring that the endianness is detected appropriately), without needing to change code in several places that calls the conversion functions. There is an additional benefit, I believe, of clarity. (the usage follows that of htonl, ntohl, etc.)


Here are other stylistic changes that I am imposing on my own code, which I also recommend for "general consumption", since they are simply good ideas:

- use std::auto_ptr to manage the lifetime of objects constructed through "new".

- use STL containers.

- more generally, use RAII ("Resource Aquisistion Is Initialization") to manage the acquisition and disposal of resources.


Finally, some more-or-less random questions about the code. If someone knowledgable about it can lend some insight, that would be great:

- In EditRoomScreen.cpp, why would this->nUndoSize ever be nonzero? I think one of the compilers that I've tried warned about a signed/unsigned comparison in the "if" statement in CEditRoomScreen::RoomChanging(), but it was not clear to me at the time what should cause the body of it to be executed.

- Clipboard.cpp appears to leak, in SetString(const WSTRING&).

- The handling of strings seems odd, to me. If I understand correctly, some effort has been taken to ensure that the in-memory representation of a string always matches (byte-for-byte) the external representation that is stored, e.g., in the database. Is there any reason not to simply convert to/from wstring for internal use? Or am I misunderstanding the situation?

- In DbRooms.cpp, near line 3660, does the c4_Bytes (it's named SettingsBytes) take ownership of the BYTE* that is used to construct it?

- the documentation for CDbMessageText indicates that the destructor will automatically "flush" (i.e., apply changes to the database). The code does not appear to do this. Which is supposed to be true?

- In CDbMessageText, if "Clear" is called, should bIsLoaded perhaps get set to true?

- Why are the o-layer and t-layer arrays in CDbRoom allocated with 1 extra char?

- when "__sgi" is defined, there are some commented-out calls to "LittleToBig" in CDbPackedVars. These appear to be necessary to me. Why are they commented out? Have I been misunderstanding the purpose of LittleToBig?

trick
Level: Legendary Smitemaster
Rank Points: 2580
Registered: 04-12-2003
IP: Logged

wmarkham wrote:
- using enumerated types, inline functions, and function templates can eliminate many of the uses of preprocessor macros that appear within DROD headers, which can potentially conflict with names used in library header files. Admittedly, system headers should not be sensitive to most of the macros that are defined, and other libraries' headers will be similar across other platforms. I have been replacing these as I encounter them.

We've done a bit of this as well.

- use std::auto_ptr to manage the lifetime of objects constructed through "new".

May be a good idea in theory, but in practice I think it would hurt performance. It certainly wouldn't help. Also, changing stuff to use this would require many little changes all over the code to fix something that really doesn't need fixing. (This is just my personal opinion -- I'm not really an authority on this, and Erik or Mike may think otherwise).

- use STL containers.

I thought we already did ?

- Clipboard.cpp appears to leak, in SetString(const WSTRING&).

Clipboard.cpp isn't used in 1.6. FlashShadeEffect.cpp is also unused, by the way.

- The handling of strings seems odd, to me. If I understand correctly, some effort has been taken to ensure that the in-memory representation of a string always matches (byte-for-byte) the external representation that is stored, e.g., in the database. Is there any reason not to simply convert to/from wstring for internal use? Or am I misunderstanding the situation?

Well, when I wrote that string code I just wanted to get the thing working (to have it done in time of the 1.6 release). The code assumes that wchars are 16-bit throughout, so it was just easier to make a Linux 16-bit wchar type in stead of combing through everything and change it to allow 32-bit wchars. "Easy" being a relative term here, of course, since libstdc++ looks down upon strings of anything not char or wchar_t (actually there was no built-in support at all until I mentioned it at the libstc++ mailing list) -- and that, again, is the reason for the Mess that is The String Code. To tell you the truth, I'm not very fond of it. In fact, I really dislike it, because of the mess required to create a WSTRING or string of WCHARs. Grrr.

- Gerry

mrimer
Level: Legendary Smitemaster

Rank Points: 5056
Registered: 02-04-2003
IP: Logged

wmarkham wrote:
- In EditRoomScreen.cpp, why would this->nUndoSize ever be nonzero? I think one of the compilers that I've tried warned about a signed/unsigned comparison in the "if" statement in CEditRoomScreen::RoomChanging(), but it was not clear to me at the time what should cause the body of it to be executed.

nUndoSize is the size of the sequence of changes that have been made to the current room when the room editor screen is entered. When playtesting the room via F5, the room is saved in its current state and the sequence of room changes persists. So, when returning from playtesting, the undo list's size is marked. This is a simple convenience for the user: when they undo/redo room changes, the room won't have to be resaved if the undo list is the same size as it was the last time the room was saved, since the state should be identical. The only exception to this is when manual modifications are made after some of these retained changes have been Undone. In this case, the remembered size is reset (i.e. now all bets are off as to whether room state will ever be identical to what it was the last time it was saved). Does that make sense?

- Clipboard.cpp appears to leak, in SetString(const WSTRING&).

Thanks for reporting this. I've fixed it in the 1.6 and 2.0 code.

____________________________
Gandalf? Yes... That's what they used to call me.
Gandalf the Grey. That was my name.
I am Gandalf the White.
And I come back to you now at the turn of the tide.

wmarkham
Level: Master Delver

Rank Points: 125
Registered: 12-06-2004
IP: Logged

mrimer wrote:
Does that make sense?

Yes, it does. Thanks!

wmarkham
Level: Master Delver

Rank Points: 125
Registered: 12-06-2004
IP: Logged

trick wrote:

- use std::auto_ptr to manage the lifetime of objects constructed through "new".

May be a good idea in theory, but in practice I think it would hurt performance. It certainly wouldn't help. Also, changing stuff to use this would require many little changes all over the code to fix something that really doesn't need fixing. (This is just my personal opinion -- I'm not really an authority on this, and Erik or Mike may think otherwise).

I certainly agree that there is no need to mess with something that already works. That's reason enough to avoid making changes.

I suspect that there may be a misconception on your part about the performance impact of using auto_ptr, however. Keep in mind that each pointer that is managed by auto_ptr has been allocated by "new", and will eventually be passed to "delete". As a developer, I hardly expect dynamic memory allocation to be fast to begin with, in the sense that I often go out of my way to allocate all the dynamic memory that is needed before any performance-critical loops, and wait until the end to deallocate it. It is hard to imagine how a simple class like auto_ptr could add any unacceptable performance penalty to it. (The performance-critical code should continue to work with raw pointers, if it is not going to allocate or deallocate them anyway.)

In regard to changing things all over the place, I would like to point out that you could start out by simply replacing one "delete" at a time. In most cases, this is simply a matter of finding the place in the function in question where it "gains ownership" of the pointer, and constructing an auto_ptr at that point. When it goes out of scope, the "delete" will occur. Occasionally that can change the timing of the "delete", but if that is really an issue, just add a call to "ptr.release()" at the point where you would like the "delete" to occur. Changing the intervening uses of the pointer to use the auto_ptr instead can be optional.

- use STL containers.

I thought we already did ?

Yes, you do. However, I still see a number of container-like data structures (the monster list and tile arrays in CDbRoom spring to mind) that use hand-rolled linked lists or C-style arrays. Of course, "if they ain't broke don't fix 'em", probably applies here, but in my opinion, the STL containers have so much going for them that, from a code maintenance point-of-view, the hand-rolled solutions do seem broken by comparison. (As with std::auto_ptr, the ease of memory management alone is a convincing rationale to me.) But that's just me!