Bobdevis' Diablo and Hellfire project

[bobdevis]

All right, first public release!
http://sourceforge.net/projects/projectddt/

The bad news is that it's Linux-only at this point.
The good news is that the installation instructions are not more complicated then those of some ScummVM games.
Binaries for x86 and x86_64 provided.

[DrMcCoy]

Not to sound "ungrateful", but I for one would prefer a proper repository with history. Always very interesting and enlightening how people organize their commits.

Did you, perchance, properly manage your code with git, hg, bazaar, what-have-you?

[bobdevis]

Nop, sorry. What you get is just a bunch of packed plain-txt files and some binaries, basically. I am learning this as I go along.

When I started I didn't even know how to properly use static c++ functions.
How to effectively run an open source project is just another skill I will have to pick up.....

[DrMcCoy]

Well, if I can give you some advice, open a github account, read some git for beginners tutorials and go along with it.
The history won't be as "clean" as it could be, you'll basically start with a big "Initial commit" commit, but it's better than nothing.

Maybe it's just me and the git-koolaid I've swallowed, but I couldn't live without git anymore.

[bobdevis]

Sounds good, will do.

[md5]

I had issues with the link you provided for the Hellfire patch. I managed to download it from here:
http://www.ladyofthecake.com/diablo/howto.htm

Concerning the questions you got inside the README file:

The complete patch notes for Diablo are found in Blizzard's site:
http://us.blizzard.com/support/article. ... leId=21119

Regarding libMPQ: There is an alternative library for MPQ file manipulation, StormLib:
http://www.zezula.net/en/mpq/stormlib.html

It does run under a variety of platforms, though I'm unsure if it's endian-safe

As for a Smacker decoder... you can use the one in ScummVM (check the /video directory)

Concerning the .r files... did you create these yourself?
Are you rewriting all of the game logic from scratch?

Also, I just had a quick look at the code...
Some observations I have (note that these are merely pointers, given out with the best of intentions!). There are places where you're trying to mimic functionality that C++ already has.

Here are some bits I saw:

First, try to change include files so that they have a .h extension (e.g. you're including levy_mul_primitives.cpp inside levy_mul_utils.cpp and levk_tsc_texmaker_cel.cpp inside levk_tsc_texmaker_pcx.cpp).

You can simplify char_cool() inside ddtblob.cpp using strchr:
http://www.cplusplus.com/reference/clib ... ng/strchr/

i.e. something along the lines of:
char *characters = "0123456789abcdefg(...)";

if (strchr(characters, in))
return true;
else
return false;

Concerning char_to_int() inside levx_mul_utils.cpp: you can use atoi() for that:
http://www.cplusplus.com/reference/clib ... dlib/atoi/
For char_to_int_hex(), you can use a combination of atoi() and itoa():
http://www.cplusplus.com/reference/clib ... dlib/itoa/

Same for didget_to_string(), you can use itoa() instead

Concerning is_a_sane_savefile_name(): you can use isalnum() for that:
http://www.cplusplus.com/reference/clib ... e/isalnum/

power_of_two() could be rewritten to something like:
ceil(x / 8 ) * 8 or even better ceil(x / 8 ) << 4

The stuff inside textline_get_width_and_number() could probably be moved into an array

Also, to compare strings (e.g. inside levg_fsy_loadsave.cpp) you can use strcmp():
http://www.cplusplus.com/reference/clib ... ng/strcmp/
or you can use the String class and use functions like substr():
http://www.cplusplus.com/reference/stri ... ng/substr/

You might also be interested in Boost:
http://www.boost.org/

Finally, the file names are a bit confusing... e.g. levm_mul_tile_operations.cpp. Why "levm" and "mul"?

[bobdevis]

Thanks for checking it out, MD5.

Regarding libMPQ: There is an alternative library for MPQ file manipulation, StormLib:
http://www.zezula.net/en/mpq/stormlib.html

Yeah, I know, they share code. Both of them blindly assume Intel.

As for a Smacker decoder... you can use the one in ScummVM (check the /video directory)

Yeah, I know you have one, its a bit of a clumsy request since I don't have a repository yet, but I was hoping someone else with video codec experience could look into that at some point.

Concerning the .r files... did you create these yourself? Are you rewriting all of the game logic from scratch?

Yes, created the .r (stand for "rules") files myself. I can't really post the stuff I used to create the big ones because of copyright.
Will have rewrite the game logic as pretty much everything is hard-coded, or has hard-coded exceptions.

Finally, the file names are a bit confusing... e.g. levm_mul_tile_operations.cpp. Why "levm" and "mul"?

'levm' implies a tier in the include chain. levm classes can not refer to levl classes, but can 'see' all levn classes.
'mul' = multiple = classes in there are uses by more then one thread.

The complete patch notes for Diablo are found in Blizzard's site:

Thanks but that is not it. I want the game credits, not the patch notes. The stuff that scrolls overs your screen when you finish the game.
Blizz added like a 1000 names to that list in later patches (most of them Battle.net testers).
I want all those names (or a way to extract them). I don't want anybody angry.

power_of_two() could be rewritten to something like:
ceil(x / 8 ) * 8 or even better ceil(x / 8 ) << 4

I was under the impression you should stay away from bit-shifts to stay endian-independent?

[all the other stuff]

Cool, thanks.

[DrMcCoy]

I was under the impression you should stay away from bit-shifts to stay endian-independent?

Not really. Shifting works the same, no matter the endianness.

md5's suggestion isn't really optimal either. Better have a look here: http://graphics.stanford.edu/~seander/b ... UpPowerOf2

'levm' implies a tier in the include chain. levm classes can not refer to levl classes, but can 'see' all levn classes.
'mul' = multiple = classes in there are uses by more then one thread.

That's...not really that nicely organized.

You should probably rather organize them by what they do, and group them in related directories. Look at ScummVM's source tree, for example.

In general, for classes, you should also split the interface (class and method declaration / method prototypes) from the implementation (method definition / the actual code). Put the former into a .h (or .hpp) file, the latter into a .cpp file.

I.e. your foobar.h file would contain this

Code: Select all

#ifndef FOOBAR_H
#define FOOBAR_H

class Foobar {
public:
  Foobar(int foo);
  ~Foobar();

  int getFoo() const;

private:
  int _foo;
};

#endif // FOOBAR_H

While your foobar.cpp file would contain this

Code: Select all

#include "foobar.h"

Foobar::Foobar(int foo) : _foo(foo) {
}

Foobar::~Foobar() {
}

int Foobar::getFoo() const {
  return _foo;
}

Another thing:
Don't do "using namespace std;"

Importing a whole namespace is asinine. Either import only the stuff you need ("using std::cout;") or just get over it and prepend the namespace where you need it.


EDIT: Another, another thing: Your code currently leaks like hell. For example, ui_border allocates a lot of stuff (unecessarily, too, you could just directly make the member variables arrays or use std::vector) in the constructor and never frees it.

EDIT 2: You actually don't have any destructors at all. Bad. The general idea in C++ is that you allocate your resources (as far as possible) in the constructor and free them in the destructor. C++ has no garbage collector, if you don't free your resources, your program will leak. I.e. it will just eat more and more memory until everything is gone. In which case, on Linux, the OOM killer will wreak havoc on your system.

Since you run Linux, you should probably look at a great programm called valgrind. Its memcheck tool can help you immensely in finding bad memory access and, very important, memory leaks.

EDIT 3: It looks like you're not that familiar with C++ in general, right? Have a look at these books. I for one found them quite enlightening when I made them jump from C to C++.

[bobdevis]

Just to be clear: I am explaining what I did, not saying its a good choice:

That's...not really that nicely organized.

As it is now, I only use 2 header files and they are basically just holding a lot of integer and float name defines.
For the rest the whole thing is header-less at the moment. It has been nice because you can change stuff faster this way.

EDIT: Another, another thing: Your code currently leaks like hell. For example, ui_border allocates a lot of stuff (unecessarily, too, you could just directly make the member variables arrays or use std::vector) in the constructor and never frees it.

[/quote]
Bad code? Oh yes, I did that some time ago before I figured out you can just assign an array size at declaration time.
Leak? Don't think so. I only create ui_borders (and all the other ui elements) ONCE and never drop them. The idea is that they all have to exist all the time so that the render-thread can read the ui-stuff lockless.

Compared to the texture data (that IS unloaded/deleted) the memory needed to hold all the ui offsets is pretty negligible.
[/quote]

You actually don't have any destructors at all. Bad.

All the elements that can unloaded during runtime do get deleted(unless I forgot something). Its just that I do it manually with the "delete" or "delete[]" commands and my destructors are generally called "kill()".

After loading a game level like 30 or 40 times the mem usage stabilises at some point.
You might think it leaks a lot at first when you run it because there is a allocation burst when a level is loaded.

[DrMcCoy]

For the rest the whole thing is header-less at the moment. It has been nice because you can change stuff faster this way.

That's neither true nor how it works. At all.

The idea is that they all have to exist all the time so that the render-thread can read the ui-stuff lockless.

Misconception: It's not a given that all used resources are automatically given back to OS when your program quits. Sure, that's true for most (all?) modern OS, but not a given. You still should manually free all your resources on quit.

Compared to the texture data (that IS unloaded/deleted)

Frankly, I wouldn't trust that code to free all resources correctly. You really should familarize yourself with valgrind, it's the best thing since cookies.

Also, read my later EDITs in the previous post, if you haven't already.

[bobdevis]

That's neither true nor how it works. At all.

Could you maybe elaborate that?
If you have a header you have to maintain the h AND the cpp file.
If you have a system that works in a header-less, linear include-chain then that drastically reduces the amount of files you have to keep up to date, right?

All the tutorials on header files I have seen didn't really show me any advantage to using them if you just can manage to link linearly instead.

[DrMcCoy]

If you have a system that works in a header-less, linear include-chain then that drastically reduces the amount of files you have to keep up to date, right?

You normally don't change header-files all the time. You often normally prototype your class (and by class, I mean actually one small-ish self-contained puzzle piece, if you will) in a header file. Then you implement that interface in the cpp.
Not that it really is any kind of effort changing a method signature in two places. o_O

It gives a logical separation between all the different pieces in your program, all self-contained. In theory, you should even be able to exchange one piece by something providing the same "service" in a different manner.

While what you have is a big blob of entangled spagetthi code. Sorry.

I'm not saying that to bash you, by the way, just to give you an idea how...outlandish your idea of code organization is. Like I said, look at how, for example, ScummVM does it.

Your code also forces you to recompile nearly everything should you change something. While with a clear header/source file distinction, when you change the implementation of something in a .cpp file, you only have to recompile that file and link the resulting object file with the unchanged object files. Probably not that import yet for your project, but compile time really explodes quickly.


EDIT: Here's a quick valgrind log. Started a new game with a new hero and left it again without doing anything:
https://gist.github.com/7f5aa52175b655ca16e2

It also says that about 25kb are definitely leaked, about 30MB possibly lost and still reachable.

[bobdevis]

You normally don't change header-files all the time. You often normally prototype your class (and by class, I mean actually one small-ish self-contained puzzle piece, if you will) in a header file. Then you implement that interface in the cpp.

The problem is that for that to work smoothly you need to know how you code will look like before you start
The idea was to roll header-less until the amount of features stops growing that fast. THEN pour the mess into some formal constraints and start proper documentation.

Your code also forces you to recompile nearly everything should you change something. [...] but compile time really explodes quickly.

Ah, there is a 'solid' advantage of headers, thanks.

While what you have is a big blob of entangled spagetthi code. Sorry.
I'm not saying that to bash you, by the way, just to give you an idea how...outlandish your idea of code organization is. Like I said, look at how, for example, ScummVM does it.

No problem I don't mind being roasted by the pro's.
Besides, I was not really able to comprehend most of ScummVM code due to the very high level of abstraction of it.

EDIT: Here's a quick valgrind log. Started a new game with a new hero and left it again without doing anything:

Oooh now that is nice development tool will definitely use in the future.

Anywy, I see you have the data files and ran the code.
Could you at least confirm that the thing runs stable on your machine......

[DrMcCoy]

The problem is that for that to work smoothly you need to know how you code will look like before you start

You should have a general idea how to do something, yes.

You can of course try things out, change and scrap them later. In general, it's not that much of a hassle to change both the .h and the .cpp. And really, if you come back later to a piece of code, a clean structure (and comments, you're mostly missing those too) is essential. It's really funny how quickly you'll forgot how something worked in detail.

The idea was to roll header-less until the amount of features stops growing that fast. THEN pour the mess into some formal constraints and start proper documentation.

Your code is already far too big for that, IMHO.
It's really better to think about how to organize something first before you code. Programming is not typing 100% of the time, it's also a good part thinking (and drawing on a piece of paper) how to do something cleanly and beautifully.
Doing that later normally doesn't work so well.


Interestingly enough, the leak report doesn't look as bad as I feared. Mostly it's just stuff that's still reachable (i.e. resources you allocate and constantly to the program the quit). Those should be clean-up too, though.
A few probably bad apples though: https://gist.github.com/d096fcbebb21612d017a

Also, your use of "kill()" instead of a real destructor is very, very weird. Very un-C++, and cumbersome, since you have to call it yourself while the destructor is automatically called when the object is destroyed (either when its scope ends or when it's delete'd).

I was not really able to comprehend most of ScummVM code due to the very high level of abstraction of it.

A high-ish level of abstraction is probably what you want anyway. The idea is that each class/object knows how to do the fiddly details, while the surrounding code just asks the object to do its thing in an abtract way. I.e. your render code doesn't need to know about character drawing and animation in detail, it just asks each character to draw itself.

Could you at least confirm that the thing runs stable on your machine......

Looks pretty stable, yes. Despite the overdose of threads, no dead-locks.

[bobdevis]

Also, your use of "kill()" instead of a real destructor is very, very weird. Very un-C++, and cumbersome, since you have to call it yourself while the destructor is automatically called when the object is destroyed (either when its scope ends or when it's delete'd).

I was weary of auto-deletion since lots of stuff gets passed from one thread to the other.
It disappears from the scope of one thread and appears in the scope of the other though the mailbox system.


Anyway, thanks again for looking at my stuff...