Critical bug in MCServer

[xoft]

Right now I have code that provides reasonable multithreading for input, loading / saving, generating and ticking; it seems to be working; also has no visible memory leaks (after a few minutes of playing).

The only problem is that sometimes the client disconnects with a "Bad compressed data format" message.

I'd like to commit this code, is anyone against such a commit?

[tbar]

I'd like to commit this code, is anyone against such a commit?

Nope!

[xoft]

Okay, no-one protests, so the code's up in the repository. Go grab it and fight it

It's still far from perfect, many things are broken, but it's a start.

Off my head, the things broken:
- generating - sometimes a chunk is generated twice, resulting in "broken" trees
- saving - when unloading unused chunks, they need not get saved
- lighting - after generating a number of chunks, lighting gets really slow and actually seems to cause clientside problems - temporary freezes
- entities - player entities don't seem to be removed from the world when the player disconnects
- altogether threadsafety - cChunkPtr is not exactly thread-safe, so while it works in 99 % cases, it fails horribly in the rest 1 %.

Still, I think there are some key concepts in the commit that are worth pursuing:
- locking, locking, locking
- letting objects manipulate themselves instead of "external godmode" (ForEachClient() instead of GetAllClients())
- pluggable world storage (cWSSCompact class reads and writes the PAK files, more schemas may be introduced and user-specified)
- decouple objects that should not be coupled (cChunkMap and cWorldStorage)

[tbar]

I cannot compile r251 on Linux

source/cWorld.cpp: In Constructor cWorld::cWorld(const AString):
source/cWorld.cpp:180:63: error: cannot pass objects of non-trivially-copyable type const AString {aka const struct std::basic_string<char>} through ...

EDIT:
I did a small change to the offending line mentioned above to get it to compile. It now gives errors in LeakFinder(cpp|h) regarding windows.h. I tried placing these in #ifdef _WIN32 but ended up with something I cannot remedy. It looks to be code specific for Windows -- which is why it cannot compile on Linux -- but I cannot say for sure.

[FakeTruth]

Cool !!

One thing I noticed is that block entities (furnace, chests, etc.) do not work

EDIT:
Nevermind, I see you commented it out because it's dangerous :O

[xoft]

It now gives errors in LeakFinder(cpp|h) regarding windows.h.

Oops, sorry for that. LeakFinder and StackWalker need to be specifically removed from the GNUmakefile; I tested with the old makefile so I didn't notice. They are only for MSVC builds, and only for Debug ones.

I'll fix that soon(ish)


About block entities - any interface (public function) that returns pointers that are handled and owned internally by an object is potentially unsafe because of multithreading. We do need to get rid of each such function and make it work differently.
Unfortunately this would mean breaking a lot of Lua stuff, so I decided to just comment it out for the time being, hoping that someone with more knowledge of Lua can provide a workaround around this.
Worst case scenario - we'd need to hand-implement Lua-specific class for each MCS object that Lua can use and map Lua calls to internal calls.

[FakeTruth]

So wouldn't a UseBlock( coords, player ) function in cChunk or cWorld be sufficient?

[xoft]

That's exactly what I'm after - functions that do something on an internal object, instead of publishing the object.
I'm not sure though if one function will be enough, haven't really looked into what operations are done to the entities. Each operation will eventually need a function.

[FakeTruth]

How about using some sort of callback?

Code:

main()
{
    UseBlockEntity( OpenChest, x, y, z );
}

void OpenChest( cBlockEntity* a_Entity, cPlayer* a_UsedBy )
{
    Bla b = a_Entity->GetBla();
    a_UsedBy->DoStuffWithBla( b );
}

I'm not sure how practical this is, but it would work with Lua

[xoft]

Callbacks are a great way of doing things, though I'd prefer class-callbacks (see cWorld::ForEachPlayer() for an example). Instead of passing a function pointer, you pass a pointer to a class that has overridden a specific virtual method that does the actual work. The advantage is that the class can carry more data (parameters, accumulators etc), the disadvantage is that probably this won't work with Lua (I have no idea).