[Shader-Based Rendering]

Sweet roller! That's got to be the single biggest bang for the buck in recent memory - you improved the framerate by a factor of ~10

(reflect/refract/patches now take 1.4/1.6/0.7 ms)

Allocations are indeed expensive on Windows - VirtualAlloc is inexplicably slow (well, it does zero out the pages) and HeapAlloc is quite wasteful for small allocations, not to mention crazy complex (2 indirect DLL calls, >= 12 sub-functions and hundreds of instructions, including at least one with LOCK prefix).

That said, there is considerable room for improvement in pool_alloc (inlining, removing the assertions in release mode). Since it is now being called millions of times, I think it's worth tackling. Let's talk about that in tomorrow's meeting.

[Compile Errors]

Sorry about the delay, things are hoppin'.

Please use [ php ] tags for source code and refrain from posting so many identical gdb warnings ("no debug information available") - that really floods this thread.

I see two problems with the code as posted: the debug_printf format string contains %1s (the digit one) instead of %ls (the letter L), and:

if(pfile) // preserve a guarantee that if pfile then we either return an error or set *pfile
debug_printf(L"not found: %1s\n", subdirectoryName.string().c_str());
return ERR::VFS_DIR_NOT_FOUND;

needs to be changed to:

if(pfile) // preserve a guarantee that if pfile then we either return an error or set *pfile
{
	debug_printf(L"not found: %1s\n", subdirectoryName.string().c_str());
	return ERR::VFS_DIR_NOT_FOUND;
}

After recompiling, please post any lines from the terminal output containing those "lookup:" or "not found:" strings we just added.

[Shader-Based Rendering]

hm, looks like performance has indeed taken a hit.

The profile says each of the three passes (reflections, refractions, patches)

takes about 15 ms, basically all billed towards "compute batches".

This is on the default map at maximum zoom on

Win7  (6.1.7600)
Graphics Card  : NVIDIA GeForce GTX 460
OpenGL Drivers : 4.1.0; nvoglv64.dll (8.17.12.6099), nvoglv32.dll (8.17.12.6099)

Vertexshader is about half as fast as shader (after switching at runtime).

However, fixed is about twice as fast as shader, with only 3..8 ms spent in "compute batches" (the large variance can be influenced by alt+tab / interacting with other windows that overlap the game).

[File.cpp(132): Function Call Failed: Return Value Was -110301 (Error During Io)]

Thank you for this log, it is very helpful.

I recognize events from various stages of GameSetup.cpp's Init():

InitVfs

g_Logger = new CLogger;

CNetHost::Initialize();

CONFIG_Init(args);

and - crucially - nothing afterwards. This means our very first attempt to load a file fails, which should make this much easier to reproduce.

It may be important to set up one or more FAM requests via dir_watch_Add, but otherwise, a simple File::Issue and FileImpl::WaitUntilComplete ought to cause the breakage.

open("/home/ugo/Projects/0ad/binaries/data/config/default.cfg", O_RDONLY) = 6
sched_getparam(8986, { 0 })             = 0
sched_getscheduler(8986)                = 0 (SCHED_OTHER)
rt_sigprocmask(SIG_SETMASK, ~[RTMIN RT_1], [], 8) = 0
mmap(NULL, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0) = 0x7f9056976000
mprotect(0x7f9056976000, 4096, PROT_NONE) = 0
clone(child_stack=0x7f9056978fb0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x7f90569799d0, tls=0x7f9056979700, child_tidptr=0x7f90569799d0) = 8990
rt_sigprocmask(SIG_SETMASK, [], NULL, 8) = 0
futex(0x7fff99caa97c, FUTEX_WAIT_PRIVATE, 1, NULL) = 0
write(1, "file.cpp(132): Function call fai"..., 80file.cpp(132): Function call failed: return value was -110301 (Error during IO)

And this is supposed to be lio_listio and aio_suspend? So the horrible old glibc emulation of aio via threads is still in place? Dear lord.

[File.cpp(132): Function Call Failed: Return Value Was -110301 (Error During Io)]

hm, Philip has wow-ed me before with an strace log listing each syscall and the parameters.

Unfortunately that doesn't seem to have worked here - it just shows the usual program output (or did we get the wrong file?)

I am relieved to hear "previously working builds failed", i.e. it isn't due to recent changes and is probably an OS bug. This is actually plausible because I don't think many programs use aio.

Are you willing and able to report this bug in your distro? Not sure which channels are appropriate there.

It'd be interesting to just call those basic file APIs on existing files - it doesn't seem to be failing immediately, since there is mention of texture conversion in the backtrace and we open/read other files first (several config files and then hwdetect.js). If we get a relatively simple reproducible test case (probably requires reading multiple files) showing the problem, it's much more likely to be fixed.

[File.cpp(132): Function Call Failed: Return Value Was -110301 (Error During Io)]

Thanks for that info!

So we have a call to aio_error returning EINPROGRESS, then successful aio_suspend, but aio_error returning EPERM. That's weird.

There's a scary description of a race condition with a similar result:

https://partner-bugzilla.redhat.com/show_bug.cgi?format=multiple&id=595499

However, this always happens, right? (i.e. it's not a race condition)

Can you run the game in strace and post the resulting log? Maybe we'll see some obviously incorrect parameter.

Just to rule out other weirdness: this is on a plain hard disk with no special/rare filesystem, right?

[Compile Errors]

Uhoh, there were major changes to the path handling code recently to improve support for various Linux filesystem encodings. That may well have broken OS X.

Can you please add a line debug_printf(L"lookup: %ls\n", pathname.string().c_str()); to the

beginning of vfs_Lookup() at vfs_lookup.cpp:73, and replace "else return ERR::VFS_DIR_NOT_FOUND;"

with

else
{
	debug_printf(L"not found: %ls\n", subdirectoryName.string().c_str());
	return ERR::VFS_DIR_NOT_FOUND;
}

and let us know the output (should also be displayed on the terminal)?

[File.cpp(132): Function Call Failed: Return Value Was -110301 (Error During Io)]

Thanks for the report; aio_return failing is bad news. What kind of disk is this on?

Can you tell us the value of errno at that spot in the code?

Finally, could you please add some instrumentation to check what the actual return values of

aio_error and aio_suspend were?

I'd recommend replacing the while loop at file.cpp:124 with something like:

int error = 1234, suspend = 1234;
for(;
{
	error = aio_error(&req);
	if(error != EINPROGRESS)
	{
		debug_printf(L"aio_error %d %d\n", error, errno);
		break;
	}
	aiocb* const reqs = &req;
	suspend = aio_suspend(&reqs, 1, (timespec*)0);	// wait indefinitely
	debug_printf(L"aio_suspend %d %d\n", suspend, errno);
}

and adding a [debug_]printf of errno and the return value of aio_return.

[Reducing Boost Dependency]

Per today's meeting, we're going to introduce NativePath. Any code that receives filenames from external sources must ensure they don't contain illegal /: nor non-ASCII characters. NativePath is just a std::wstring (on Linux, each byte is stored in a wchar_t). This allows easy processing within our code and also makes for correct display of the path (even without knowing about the actual FS locale) in the common case of ISO 8859-1.

The corresponding patch is attached. Works on Windows; would appreciate a careful look (you never know) and testing on Linux.

reduceBoost2.patch

[Reducing Boost Dependency]

QUOTE

Native filesystems don't handle any encodings, they just handle strings of '/'-separated bytes.

OK. So in your example, we are told of /home/josé and aren't supposed to care how it's encoded. José wants to save a game and chooses as the filename "josé". Now unless we understand how to translate the unicode string he told us into the actual FS encoding, the filename will turn out to be some gobbledygook from the perspective of the OS. Even if it turns out to be legal, he might not be able to copy that file.

QUOTE

If we implement locale support, I don't think there's any need to try UTF-8 first - the locale is more authoritative so we should just use that.

Since the locale might turn out to be UTF8? But you were saying the locale might be set incorrectly, so I do think there is value in providing the UTF8 fallback (under the assumption that some Linux FS are actually UTF8 encoded despite the locale being set to es-ES or whatever).

[Graphics hardware compatibility]

I like the sound of that. Of the 2% lacking fragment_program, some are also way below the min spec we established years ago (Rage 128? LOL)

So: good to have the data; it looks like this decision now rests upon a wide enough survey.

[What Does Everyone Think Of Switching To Git/github?]

When I last used it (right about the time Philip started the HG clone), TortoiseHG was a rather rough-around-the-edges shadow of its TortoiseSVN counterpart. The TortoiseGit screenshots look a little nicer, but there is still the risk of alienating users (Git being quite a different model).

My sympathy for DVCS is definitely increasing - there have been several network outages (more of a problem for SVN), glitches (tree conflict and HTTP 405 that require revert+delete+update+try again, ugh) and better merging would indeed be good.

However, I agree with Philip that we can already have most of the benefits (via mirror), that this particular situation and others foreseeable in the near future don't require DVCS, and also believe that switching mid-project is to be considered very carefully. Would the significant effort of completely switching over be recouped by the increased efficiencies of Git? Doubtful at this fairly late stage.

[Reducing Boost Dependency]

Either sounds fine to me.

Alright, I'll go with Path::Join/Path/Extension, and FileExists/FileSize can stay in the root namespace.

I think your dream is to just "get" whatever the OS gives us in the way of paths and treat them as a black box. However, we are going to want to append paths, etc. - so we can't just pretend to accept any encoding.

That is what I want (in an ideal world where we have time to not just kludge it ). If we want to append/split/etc paths from the OS, we don't have to know the encoding, since we still know e.g. the code unit '/' is the directory separator regardless of encoding and regardless of code unit size (given that Linux will only use ASCII-compatible encodings).

OK, this is the part I don't get. Granted, path components are separated by '/' - but do the filesystems successfully handle DIFFERENT encodings of each component? If we just grab them from any source and pass them on, it could be 8859-1, UTF-8 or any other CP - and a mix of them doesn't sound healthy.

If we want to append paths which come partially from our own code (mod names, saved game names, etc) it's best to just require our path fragments to be ASCII

That's the QString approach and it is PART of the solution, yeah. But again, there are various other sources of pathnames: savegame paths from the UI (this alone is enough to torpedo any attempt at being unaware of the OS/FS encoding), data files, command line and maybe others. Each of these must have a well-defined conversion to the internal wchar_t representation.

Those edge cases have to be weighed against the inconvenience of always having to go through conversion functions.

We need some platform-dependent conversion functions anyway, because std::ofstream wants char* on Linux. Otherwise it seems the most common situation is Join(NativePath, std::wstring) which can assume the second argument is ASCIIish and convert to native automatically, and LOGMESSAGE(L"Loaded %ls", NativePath.string().c_str()) which can convert to wstring by trying UTF-8 and falling back to ISO-8859-1 (it doesn't matter if that's lossy since it's just a log message).

Let me elaborate on that. It is clear we need conversion functions at the boundaries of our code (e.g. reading command line - that's exactly what sys_WideFromArgv is for). However, I propose to eliminate all other conversions INSIDE the game, which is the vast majority. All game code should be able to rely upon the fact that we have correctly encoded wchar_t strings that can be parsed/displayed/manipulated.

When someone is running the game from /home/josé (last component is 4 bytes) with locale es_ES.iso-8859-1, decoding and encoding as UTF-8 would break (either with a decoding error, or with a 3 or 5 byte re-encoded output). Boost Filesystem V2 at least decoded/encoded using the current locale, so it would work as long as the locale was configured correctly; V3 on POSIX uses byte strings instead so it should work even in misconfigured environments. (OS X often has misconfigured environments, it seems.)

OK, I propose to extend or add similar functions to sys_WideFromArgv to correctly handle that case.

If UTF8 decoding fails, try the current locale. If that also "fails" (i.e. we decode into a pathname that doesn't exist after passing back through our wchar_t -> native conversion), then require the user to specify the right locale on the command line.

(If there's anything I can do to help with this then I'd be willing - I don't mean to sound like I expect you to do everything here . If there was some NativePath class/typedef that was implemented as std::wstring using UTF-8 everywhere, I think I should be able to change that to std::string on Linux/OS X and fix up all the conversions and operations and whatnot, if that would help.)

Great It is appreciated - I still can't test on Linux and will probably end up causing a bit of breakage, as already noted :/

[Game Is Very Slow - Due To Aken64 Not Loaded?]

Let me just confirm that the absence of aken64 is not a problem.

Unfortunately there have been further changes in the code signing process and this driver no longer loads despite proper (I think) signing with a Fraunhofer certificate, which is trusted by a Deutsche Telekom certificate that should be included in Windows.

If anyone is knowledgeable on this particular issue, I'd very much appreciate some pointers on what we're doing wrong, or how to do it right.

[Collecting data from users]

Looks like Philip now has enough data to make an informed decision on the graphics paths (requiring fragment_shader or _program is apparently OK).

http://www.wildfiregames.com/forum/index.php?showtopic=14419&pid=217338&st=0entry217338

I've also noted and fixed some problems with the CPU and cache detection code based on "weird numbers" in these reports. All in all, a very useful tool! Big thanks to Philip for implementing it in such an awesome fashion

[Reducing Boost Dependency]

Excellent points, as always

I think a namespace is important because otherwise the function names are misleading, e.g. I'd probably expect Join("a", "b") to be equivalent to "a"+"b" and wouldn't expect it to be path-related.

Agreed.

Boost doesn't use the namespace "fs", so I don't think it'd be confusing to have something silently Boost-like (not advertised as a drop-in replacement, but preferring compatibility when possible) reusing that namespace. Anybody new to the code would have no expectation that it's like Boost, and anybody old will be aware of this change.

OK, but these functions are in path_util and only cover that functionality; fs is kind of exaggerating there.

How about Path instead? or PathUtil even, though that's getting rather clunky.

SHARED_PTR looks very ugly. Can't it just be shared_ptr? (and include using 'using' or 'typedef' instead of macro)

Yeah, that's one of the first things I did. Turns out only one kind of shared_ptr is needed, the concerns of having to mix them are mooted, so we can revert to the previous just-put-it-in-main-namespace approach.

unordered_map exists there too, instead of using STL_HASH_MAP which has much less predictable behaviour? (Regardless of that, outside of lib we can rely on Boost and should probably get rid of STL_HASH_MAP entirely and use unordered_map, for modernity and consistency. That's probably separate from this patch, though.)

hm, I expect so, but would rather get this working ASAP, so it'd be good to address that later

I don't remember ever accidentally mixing VFS and non-VFS paths, such that the type system would help. But I do find VfsPath very useful as documentation, since it stops me getting to the stage of accidentally mixing paths, so it sounds good to keep it.

Agreed.

This brings up the old encoding problem - paths on Windows are natively 16-bit strings, paths on Linux are natively 8-bit, and conversion can be lossy.

Oh no, that one again

I think your dream is to just "get" whatever the OS gives us in the way of paths and treat them as a black box. However, we are going to want to append paths, etc. - so we can't just pretend to accept any encoding. Given that, we may as well prescribe one internal representation and ensure we can do all the processing we want. That also seems less prone to "oops I can only test on one OS" breakage

with suitable handling of string literals etc (which we can probably assume are ASCII)

Yes, but something as simple as möd nämes on the command line, and we have the encoding problem.

Do you have concrete examples of lossy conversion that are actually a problem? I'd be surprised to hear people wanting/expecting non-UTF-16 characters in paths.

Those edge cases have to be weighed against the inconvenience of always having to go through conversion functions. My first patch actually simplifies the code in a lot of cases because lots of conversions/casts go away.

If we want to fix things quickly, we can assume most people on Linux have UTF-8-compatible paths (which we mostly assume already), so path_from_wpath just does UTF-8 encoding, and for std::ofstream we can add a native_path_from_wpath that encodes (on Linux) or returns the std::wstring (on Windows), and it probably won't break for many people.

I do like that option, but have a serious backlog and might not get around to it today

[Reducing Boost Dependency]

The Boost stuff is actually mostly header-only, so that is basically what is already happening.

However, the problem is that (say) sys_get_executable_name has fs::wpath as part of its signature.

That would mean it can't be used as part of the codebase here, since Boost (or even a subset, which is basically the same thing, since using one header is tantamount to including the whole kitchen sink and pot of soup) is "verboten".

To fix this, I've changed that function (and all others) to return a std::wstring, and if 0ad needs a fs::wpath, it's easy to create one directly from the string. Those "easy" changes do add up, though, and I'd like to be sure it's not too much of a burden for the rest of the code from y'all's perspective.

[Reducing Boost Dependency]

We've been integrating two codebases at work and the Boost dependency in our lowlevel code is unfortunately a no-go.

I've gone through some gyrations to allow 0ad to continue using Boost while completely removing it from the other codebase.

The biggest changes are replacing ::shared_ptr with a SHARED_PTR macro (as with STL_HASH_MAP, which also requires a different namespace depending on compiler), and replacing almost all fs::wpath with std::wstring.

This snowballed into several hours' work and rather extensive changes, so I would like to hear if anyone has objections and possibly change things to ensure everyone is OK with this change.

The proposed 223 KB patch is attached; I'd appreciate if someone could test on Linux.

Some talking points:

1) exists has been replaced with DirectoryExists / FileExists. Since stat() must only be called for files, I think this is a worthwhile change to make in any case.

2) other fs:: functions have been replaced with pretty much the same functions in path_util.h. fs::wpath.leaf() and branch_path become Filename() and Path(). These are rather generic names, so I wonder if there would be value in putting them in a namespace. One route would be to call it fs:: as before and try for mostly drop-in compatibility with Boost filesystem - but I don't really like that idea because "mostly" leads to false expectations. We could also call it Path.

3) the convenient fs::wpath operator/ becomes Join(), which is available for 2 and 3 arguments. Amusingly enough, the new code is often shorter because less casts and .string() fluff arise.

4) VfsPath tried to increase safety by making it harder to confuse different types of path (real POSIX vs. VFS). However, that is very, very weak due to the implicit conversion from std::wstring, and because we're already calling fs::wpath::string() in many places. I don't see any harm in just converting to std::wstring, but it's probably good to keep the VfsPath as a typedef for added documentation.

Any other comments / suggestions?

If not, I'd like to commit in a few days at the latest, because it's rather difficult to keep 2 codebases in sync.

reduceBoost.zip

[Working On Upgrade To Premake4]

Good news.

About pushing stuff upstream: one upside of the rapid development is that others might have the same need, and it might get taken care of automagically So waiting sounds good; as Philip says, living with a custom version of Premake is fine for the mid-term at the very least.

As for VC2005 support, I agree it's no longer needed, but IIRC the only difference is the version number at the top of the XML files.

I was recently suggesting to Philip that we only support VC2010 for simplicity (one big upside is avoiding the .manifest hell). Since the hwdetect framework now also reports the compiler used, we'll soon see how many people still need VC2008.

[[Fixed] Compiling Errors - Wglext Headers]

Yeah. The problem was updates to glext.h that are fine on Linux, but require adding extra typedefs to wgl.h (since we don't want to include windows.h, we have to re-define any types used by glext.h).

[Graphics hardware compatibility]

I agree that it'd be good to cut down on the complexity of the renderer; let's just weigh the cost/benefits of each:

Proposal #1: Always require >= 4 TMUs

This one's hardly controversial - Radeon 7500 was introduced in 2001 and GF4MX is a joke of a card.

Proposal #2: Don't support shadows without GL_ARB_shadow and GL_ARB_depth_texture.

Agree with Erik, the game works well enough without shadows, so no problem there.

Proposal #3: Always require GLSL vertex shaders

This is where it starts to hurt. 945G is only 5 years old and still quite common in the Unity survey.

I think it's also worth keeping the fixed-function fallback in case of driver trouble/bugs, and to support integrated/crappy discrete graphics commonly found in laptops.

Proposal #4: Always require GLSL fragment shaders

Also questionable per the above argument. 945G is reported to have problems with shaders. Weren't you using a 945 until recently? Did it work OK? I think that's one platform that's definitely worth supporting.

[Fog rendering]

I too like the blending. It should be much less jarring than suddenly popping the whole building into

full visibility.

AoK was a software renderer, and they need to keep runtime decisions to an absolute minimum, so it may just have carried over to AoM/AoE3 out of habit. They also didn't have as much variation in building sizes.

I don't see any gameplay problems (maybe even less, because a combination of huge buildings and small visibility might mean the center of buildings are never exposed).

[Fog rendering]

Very nice!

This is done by creating a bitmap with one pixel per map tile, then doing a single 5x5 box blur on it

May I suggest a better smoothing kernel, though? A box filter is really bad. Since you're already separating the kernel, we can get a cheap but decent integral approximation of a Gaussian via binomial filter. Given your 5 taps, the coefficients are 1,4,6,4,1, which sum up to 16 and therefore don't require conversion to floating-point nor division.

I agree about not going through gyrations to support 2 TMUs, and disabling shadows sounds like a good compromise.

[As for Ticket #685]

The Creative implementation and OpenALsoft don't ignore the loop flag. It's interesting to hear that disabling looping (which is kind-of nice for the main menu but certainly not critical) avoids that bug in their OpenAL. After you've disabled it, are there any other problems? It might be worthwhile to just turn that off on OS X instead of killing all sound, but given the amount of problems we've seen, some other issues are likely to arise.

[[Resolved] Crash On Map Load]

Just a quick comment on SDL_SetGamma - that's not a critical error and can safely be ignored (just click continue), BUT should no longer be happening in more recent builds, since we now work around the graphics driver bug that seems to cause it.