Reasons for success or failure of source ports?

[dpJudas]

Linguica said:

Maybe you should calm down, put on some music

Finally an honest musician that admits his CD is a train wreck!

[grommile]

Graf Zahl said:

But this one function had everyone stumped who so far looked at it.

Which function? I'm curious now.

[Graf Zahl]

The voxel projection code for the software renderer. R_DrawVoxel in r_things.cpp in the GZDoom source.

[Linguica]

I've long thought that the next frontier for Chocolate Doom should be a large scale refactoring / code commenting effort to make the code more readable and functional (in the programming sense) for the benefit of future hobbyists, but I'm sure fraggle would consider that massively out of scope.

[dpJudas]

Graf Zahl said:

The voxel projection code for the software renderer. R_DrawVoxel in r_things.cpp in the GZDoom source.

The lovely function that uses two conventions, one for Doom (the variables prefixed with 'da') and one for Build. Toss in some stack allocations (alloca). Then proceed to maybe use an offscreen buffer if the voxel object is translucent. Apply some float to fixed conversions, different fixed point precision between Doom and Build, and season it with bit trickery and global variables. Use drawers grabbed from Build if the offscreen buffer is not active and don't forget to abuse the "drawt" sprite drawers if its translucent.

Maybe Linguica can find something worse than a train wreck for this one. :D

[Linguica]

dpJudas said:

Maybe Linguica can find something worse than a train wreck for this one. :D

[jval]

Linguica said:

I've long thought that the next frontier for Chocolate Doom should be a large scale refactoring / code commenting effort to make the code more readable and functional (in the programming sense) for the benefit of future hobbyists, but I'm sure fraggle would consider that massively out of scope.

The Doom source is readable and easy to understand. It's not like this.

[Graf Zahl]

dpJudas said:

The lovely function that uses two conventions, one for Doom (the variables prefixed with 'da') and one for Build. Toss in some stack allocations (alloca). Then proceed to maybe use an offscreen buffer if the voxel object is translucent. Apply some float to fixed conversions, different fixed point precision between Doom and Build, and season it with bit trickery and global variables. Use drawers grabbed from Build if the offscreen buffer is not active and don't forget to abuse the "drawt" sprite drawers if its translucent.

Maybe Linguica can find something worse than a train wreck for this one. :D

It took two to create Frankenstein's monster function, though.... >)

jval said:

The Doom source is ... easy to understand.

If you know it inside out, maybe. I still remember seeing it for the first time in 1998, it looked all Greek to me. The code takes many, many years to master.

[printz]

Linguica said:

I've long thought that the next frontier for Chocolate Doom should be a large scale refactoring / code commenting effort to make the code more readable and functional (in the programming sense) for the benefit of future hobbyists, but I'm sure fraggle would consider that massively out of scope.

For that to make any sense, he should first switch to an IDE and Windows friendlier build tool. CMake seems to do the job. Last time I tried, building on Windows was broken, so I had to reboot to Ubuntu to do anything to it. And sadly Chocolate-Doom is the only truly vanilla port, very useful for anything vanilla experiment related.

[jval]

Graf Zahl said:

If you know it inside out, maybe. I still remember seeing it for the first time in 1998, it looked all Greek to me. The code takes many, many years to master.

Since I do speed Greek it isn't that hard then :)
Yes, indeed, then code takes many years to master. It's 80.000 - 100.000 lines of code. It's big. But to correct my latest post, the game mechanics are indeed complex and hard to understand. But if you focus to any point it's far more readable that eg. the source code of the build engine (written in double Dutch :D). Doom source code is split to self-explained units/filenames, no crazy C-isms and well formatted. If you focus to a function, you can actually understand what is doing. In build engine you do not even know where to look.

[Linguica]

jval said:

The Doom source is readable and easy to understand.

It's really not. Just to take one aspect, related stuff is seemingly arbitrarily spread across multiple source files for no apparent reason, partly because of BK's half-finished refactoring effort before the source release, which probably just made things worse. For instance, here's a bunch of nested functions called when calculating the player movement, along with the file they're in:

P_XYMovement                    p_mobj.c
→P_SlideMove                    p_map.c
 →P_PathTraverse                p_maputl.c
  →P_TraverseIntercepts         p_maputl.c
   →PTR_SlideTraverse           p_map.c
    →P_PointOnLineSide          p_maputl.c

The main movement function goes into the movement/collision handling library, and from there into the movement/collision utility functions sub-library, and... then BACK into the main movement/collision handling library, and then BACK AGAIN into the sub-library. From personal experience I know it can be infuriating to try and follow the program's logic and constantly have to move back and forth between files because things are not grouped together in any sort of logical fashion. And that's not even going into the code itself, which can be incredibly arcane and undocumented (quick, describe to me what intercepts are and how they work! How about openings?)

[Graf Zahl]

Linguica said:

P_XYMovement                    p_mobj.c
→P_SlideMove                    p_map.c
 →P_PathTraverse                p_maputl.c
  →P_TraverseIntercepts         p_maputl.c
   →PTR_SlideTraverse           p_map.c
    →P_PointOnLineSide          p_maputl.c

That's a bad example. After all PTR_SlideTraverse is a callback that's being passed from P_SlideMove and callbacks are not really bad design as their purpose is to customize the functionality of a utility function.

[Linguica]

For a more real world example then, here's entryway getting a free 4% speed boost on a complex map from very simple optimizations (precalculating / caching the inverse of seg lengths, and testing if a line is horizontal / vertical and using a simpler algorithm) because no one had ever really understood what the code there was actually doing.

[kb1]

Linguica said:

Maybe you should calm down, put on some music

http://i.ebayimg.com/images/g/KYoAAOSwHnFVzBWD/s-l800.jpg

The Mr. Big? Paul Gilbert, Bill Sheehan Mr. Big? Hell, I'll become a mime for an association like that! Thanks - every time I go to buy music I try to remember what's not in my collection. Now, I can scoop these up!

Hell Theatre said:

I think he got it perfectly.

He wasn't even close. I guess you didn't get it either. Which makes sense, since it was your historically-incorrect post that started it.

Hell Theatre said:

Can you even do some work on your projects without standing in awe at the glorious code id created 24 years ago?

Oh, it's tough man. Hey, if it ain't glorious, why are you here? At least I'm standing in awe.

Hell Theatre said:

Sorry, but when it comes to programming, I'd rather listen to more levelheaded voices

Promise?

[Ladna]

OK "unreadable" is probably too far. "Frustrating and goofy" is more like what I meant. I have examples:

Spoiler

Link

#define R ((8*PLAYERRADIUS)/7)
mline_t cheat_player_arrow[] = {
    { { -R+R/8, 0 }, { R, 0 } }, // -----
    { { R, 0 }, { R-R/2, R/6 } },  // ----->
    { { R, 0 }, { R-R/2, -R/6 } },
    { { -R+R/8, 0 }, { -R-R/8, R/6 } }, // >----->
    { { -R+R/8, 0 }, { -R-R/8, -R/6 } },
    { { -R+3*R/8, 0 }, { -R+R/8, R/6 } }, // >>----->
    { { -R+3*R/8, 0 }, { -R+R/8, -R/6 } },
    { { -R/2, 0 }, { -R/2, -R/6 } }, // >>-d--->
    { { -R/2, -R/6 }, { -R/2+R/6, -R/6 } },
    { { -R/2+R/6, -R/6 }, { -R/2+R/6, R/4 } },
    { { -R/6, 0 }, { -R/6, -R/6 } }, // >>-dd-->
    { { -R/6, -R/6 }, { 0, -R/6 } },
    { { 0, -R/6 }, { 0, R/4 } },
    { { R/6, R/4 }, { R/6, -R/7 } }, // >>-ddt->
    { { R/6, -R/7 }, { R/6+R/32, -R/7-R/32 } },
    { { R/6+R/32, -R/7-R/32 }, { R/6+R/10, -R/7 } }
};

Link

void
AM_getIslope
( mline_t*	ml,
  islope_t*	is )
{
    int dx, dy;

    dy = ml->a.y - ml->b.y;
    dx = ml->b.x - ml->a.x;
    if (!dy) is->islp = (dx<0?-MAXINT:MAXINT);
    else is->islp = FixedDiv(dx, dy);
    if (!dx) is->slp = (dy<0?-MAXINT:MAXINT);
    else is->slp = FixedDiv(dy, dx);

}

Link

    mo = P_SpawnMobj (x+20*finecosine[an], y+20*finesine[an] 
		      , ss->sector->floorheight 
		      , MT_TFOG); 

Link

    if ( gamemode == commercial)
    {
	skytexture = R_TextureNumForName ("SKY3");
	if (gamemap < 12)
	    skytexture = R_TextureNumForName ("SKY1");
	else
	    if (gamemap < 21)
		skytexture = R_TextureNumForName ("SKY2");
    }

But overall Wagi is right, Doom is pretty readable -- if screwy. Boom really fucked it all up, like wow. Wanna take a barf? Here you go. (Sorry Ty, but come on!!!!)

[Voros]

^OK the fourth one isn't that bad. Maybe this could have been a better solution:

    if ( gamemode == commercial)
    {
        if (gamemap < 12)
            skytexture = R_TextureNumForName ("SKY1");
        else if (gamemap < 21)
            skytexture = R_TextureNumForName ("SKY2");
        else 
            skytexture = R_TextureNumForName ("SKY3");
    }

Forgive me if I got the syntax wrong. Mainly know the basics of C (and lack the total understanding of arrays).

[Ladna]

Yeah it's not, but that broken "else"/"if" is in a lot of places in Doom. Or you'll also see a "for" loop with no braces on top of like a 100 line "if"/"else" chain. It's just ugly and dangerous, but it's not unreadable.

[fraggle]

Linguica said:

I've long thought that the next frontier for Chocolate Doom should be a large scale refactoring / code commenting effort to make the code more readable and functional (in the programming sense) for the benefit of future hobbyists, but I'm sure fraggle would consider that massively out of scope.

Not sure what you'd refactor, but adding more comments sounds like a great idea. Maybe a big comment at the top of each file that explains what the module does?

But I also wonder if the Doom wiki might be better place for this.

[Wagi]

At the very least, it would be nice to get rid of all those nasty little C89-isms and also make the code use consistent indentation and spacing.

There are also some peices of code like the screen wipe code that makes absolutely no sense at all.

[Altazimuth]

Ladna said:

But overall Wagi is right, Doom is pretty readable -- if screwy. Boom really fucked it all up, like wow. Wanna take a barf? Here you go. (Sorry Ty, but come on!!!!)

Killough seemed quite aware on his style when working on BOOM and MBF: https://github.com/Doom-Utils/historic-ports/blob/mbf/p_setup.c#L893-L908

[Arno]

Voros said:

^OK the fourth one isn't that bad. Maybe this could have been a better solution

That does looks better. Personally, I would also remove the code duplication by doing the actual texture selection in a seperate function.

const char* GetSkyTextureName(const int gamemap)
{
    return (gamemap < 12) ? "SKY1" :
           (gamemap < 21) ? "SKY2" :
           "SKY3";
}

...

    if ( gamemode == commercial)
    {
        const char* skyTextureName = GetSkyTextureName(gamemap);
        skytexture = R_TextureNumForName (skyTextureName);
    }

[kb1]

Altazimuth said:

Killough seemed quite aware on his style when working on BOOM and MBF: https://github.com/Doom-Utils/historic-ports/blob/mbf/p_setup.c#L893-L908

Regardless of anyone's coding philosophies, Killough wrote his all-on-one-line formulas to prevent the compiler from moving intermediate values to memory, choosing instead to keep them in registers. It was purely a speed optimization. He profiled a lot of those functions, and rewrote them to make them faster, at the cost of readability. Though, I have a feeling that he had no problems reading them, anyway.

So, he chose to make it good for users, crappy for developers :)

[Graf Zahl]

I remember the bad of times of such crappy compilers. How ironic that this code today often produces the opposite result and actually prevents the compiler from doing good optimization.

[dpJudas]

kb1 said:

Regardless of anyone's coding philosophies, Killough wrote his all-on-one-line formulas to prevent the compiler from moving intermediate values to memory, choosing instead to keep them in registers. It was purely a speed optimization. He profiled a lot of those functions, and rewrote them to make them faster, at the cost of readability. Though, I have a feeling that he had no problems reading them, anyway.

So, he chose to make it good for users, crappy for developers :)

He could have made it good for both at the same time. The ZDoom codebase is really full of silly optimizations where something is made ten times harder to read in order to save two multiplications per wall or plane. Half the time this stuff has a Killough comment attached to it.

If you add them all up, maybe you got a 10% speed increase totally for the entire codebase at the cost of making it unreadable. That's a very poor way to optimize because you just cut off any chance of making an algorithmic change - the kind of changes that really matter for performance.

[Linguica]

Would writing a big long one-liner rather than a few equivalent shorter lines actually make the compiler treat it differently? I thought the source code got passed through a lexer and parser or whatever and and converted into an intermediate format anyway.

[Graf Zahl]

Compilers of the 90's actually did create better code for that. I haven't used Watcom C or GCC for tests but the old Turbo/Borland-C had the same behavior that avoiding temporary variables allowed for better register use.

I also optimized code back then 'because it mattered'. But in the end the result was the same as here: The code became unreadable with its focus on micro-optimization which prevented any larger scale algorithmic refactoring.

[Maes]

Ladna said:

many people have reported running Doom on a 286

That's another obscure point that has been debunked -Doom uses the i386 instruction set, not the x86, simply by virtue of being a Protected Mode game, and using the DOS4GW PM extender, which was only for i386 CPUs and upwards:

Related: Differences between 286 and 386 protected mode.

The misunderstanding that it "runs on a 286" is probably due to people running it on a 386SX -that's still a full-fledged 386 when it comes to the instruction set, but with a crippled 16-bit data bus, and a performance that's not clearly better than a 286 in most cases.

To make matters even more confusing, there were some pin-compatible 386 SX upgrade chips that plugged more or less directly into some 286/AT mobos, or even entire daughterboards/risers that could bring things up even to the level of a 486SLC2! Of course, you'd have a "386 lite" or a "486 lite" after such an upgrade, bottlenecked by the archaic AT mobo, its slow 30-pin SIMMs and other 286-era legacy buses, and minimal amounts of cache that were (mercifully) tacked on such "upgrades".

A further possible source of misunderstanding Doom's CPU compatibility is none else than its predecessor Wolfenstein 3D: that one still used good old Real Mode and was indeed intended to run natively on 286 CPUs (not 8088/8086 however).

Interestingly, a 386 wasn't any faster clock-per-clock than a 286, at least when it came to 16-bit software. The only advantage it has was that native 386 mobos had some external Level 1 cache (Intel at least never made 386 chips with built-in L1 cache). The world had to wait until the 486, to have a truly superior internal architecture (486 was practically RISC-like in performance, averaging 1 IPC) and also for IBM PCs to be blessed with something better than the ISA bus (E-ISA, MicroBus or whatnot don't count).

[kb1]

dpJudas said:

He could have made it good for both at the same time. The ZDoom codebase is really full of silly optimizations where something is made ten times harder to read in order to save two multiplications per wall or plane. Half the time this stuff has a Killough comment attached to it.

If you add them all up, maybe you got a 10% speed increase totally for the entire codebase at the cost of making it unreadable. That's a very poor way to optimize because you just cut off any chance of making an algorithmic change - the kind of changes that really matter for performance.

There is nothing even remotely obfuscated enough in Killough's code to make it difficult to make a beneficial algorithmic change. And, once you write your nifty new algorithm, copy and paste it, then optimize the hell out of it - why not? All the player cares about is the fidelity of the gameplay, and of performance, and the player should always come first. 10% saved, here and there, means 10% more time to maintain frame rate, especially in net games.

Killough was a bit lax on commenting (which is really all that would be required). But, the thing is (my guess anyway): Killough has no problem reading code of this complexity, so I don't think he felt like it needed a lot of comments.

Academic code is nice for us humans, but, at the end of the day, I'd rather play the source that the compiler liked the best, if you get my meaning.

Linguica said:

Would writing a big long one-liner rather than a few equivalent shorter lines actually make the compiler treat it differently? I thought the source code got passed through a lexer and parser or whatever and and converted into an intermediate format anyway.

Yes, it is parsed and converted. But it takes an aggressive optimization pass to remove intermediaries, wisely dole out precious resources (registers), take redundant code out of loops, etc. There are literally hundreds of optimization opportunities being taken advantage of in today's compilers. Without those, the compiler does a stand-up job at converting your code to assembly, but misses a bunch of these techniques.

The simplest compiler simply converts the code as it has been typed.

x = y / 4;

A simple compiler might divide y by 4 and store it into x. A better compiler would multiply y by .25, or even right-shift y 2 places if the vars are integers. An even better compiler might do all of this work with registers, and never store x, instead just moving the result wherever x is needed.

Writing good optimizing compilers is a massively complex task, cause if they change the meaning of the code one small bit, they are useless.

[Rachael]

Great point of view if you never plan to add features in the future, or have other contributors in your project.

If you want to have the option to be able to expand upon code in the future though, or have others help you with it, readability is really helpful.

[Jon]

kb1 said:

Academic code is nice for us humans, but, at the end of the day, I'd rather play the source that the compiler liked the best, if you get my meaning.

I couldn't disagree more.