Too much like the real Terraria source code.
PoroCYon opened this issue · 20 comments
I was a Terraria modder and a member of the tConfig and tAPI dev teams. (I also made another modding API for 1.3, but it didn't take off.), and I can say you didn't only try to clone the game, but also the code quality of Terraria:
Blocks and items, among many other things, are represented by magic numbers instead of enums.
Terraria does the exact same thing.
There are 417 lines of block comments at the top of
TerraFrame.javathat serve as manual translation tables.
We (Terraria modders) had no such thing, so the table must be made manually (or rather, we used some code to generate it). Some methods exist (such as Item.SetDefaults) to give an Item's fields (not properties, but raw fields) the desired values. (There are no subclasses or anything, and no Item/... definition lookup tables/... .) It is an infinite sea of if (type == foo) { init(); } .... Note that it uses if without else or return, drastically slowing it down, but it got changed to a switch in 1.2. (More on SetDefaults later.) In 1.2, a Terraria.ID namespace was added containing (static) classes containing constants for easy Item/... -> numerical ID lookup as well.
Terraria entities also have a netID, which slightly differs from their type: entities with the same type have the same texture, but they can have differing netIDs. The latter defines the actual properties (max life, damage, ...). Thus, separate netDefaults methods exist as well...
So as to avoid needing to declare local variables and loop indices, all of the variables for everything are declared globally at the class level. Here is one of the several hundred lines of declarations in
TerraFrame.java
Terraria didn't do this, but the compiler mangled the variable names enough, and the combination with huge methods and the usage of next to no non-primitive types resulted in lots of occurrences of locals called num256 etc. (Yes, methods with >100 locals exist.)
Although there are a few other classes, the bulk of the code is in the God Class
TerraFrame, which spans over 6,500 lines of code.
This is nothing compared to Main, spanning several 10 000s of lines of code. It containts the global game state, global loading/unloading, >75% of all the rendering code, sound handling, ... Item, NPC, Player, Projectile, WorldGen ... are huge as well.
The
TerraFrame.init()method, which is over 1,300 lines long, actually grew so large that the Java compiler started running out of memory trying to compile it! The solution? Copy half of theinit()code into a new method, calledcodeTooLarge(), and call that frominit().
The compiler manages to live with extremely huge methods, but the decompiler chokes on them. (I don't know anything about the compilation of the original source code side of things, but some badness might happen there as well.) Item.SetDefaults is one of them, and it is split in 4 (!) separate methods, called SetDefaults1 through 4, plus the netDefaults one. (This was done by the dev team. If this wasn't the case, we (the modders) would have to fix the binary using Mono.Cecil or dnlib and edit the raw MSIL (the C# equivalent of Java bytecode) programmatically...). NPC.AI, WorldGen.generate, Player.itemCheck are insanely large as well. As of 1.3, Terraria explicitely invokes the JIT compiler on startup to make it compile everything at once, so no slowdown occurs during gameplay due to the JIT. (This does happen, however, when loading textures ingame...)
FYI, decompilation alone takes hours while eating all of your CPU and RAM.
Frankly horrifying inline data tables
Those were all over the place, especially for tiles (which didn't even have a SetDefaults method). This only got worse in 1.2.
Over 1,000 lines of filling globally declared HashMaps and ArrayLists with magic numbers and strings one by one.
Map data, localised strings, ... are all managed this way, but Recipe.FillRecipes (or something like that, my memory is a bit hazy) is the absolute worst in this regard.
The control flow is so labyrinthine that some of the code is actually indented by 23 tabs.
This is commonplace. NPC.AI, Player.ItemCheck, Projectile.Update, WorldGen.generate, to name a few.
There are random print statements scattered throughout the codebase, with helpful messages like [DEBUG2R] and [DEBUG2A].
There was zero debugging information in the Terraria binary... except for something that logged every single keypress to the console, annoying everyone who used Console.WriteLine for debugging mods.
Why use a pre-existing GUI framework for your text boxes when you can easily roll your own?
There is no (official) GUI framework for XNA (although third-party libs exist), and Terraria uses an "imgui", but without any sense of structure. It simply uses raw SpriteBatch.Draw calls etc, the current 'page' is also identified by a magic number. This got better in 1.2.3 (iirc, but it could as well be 1.2.0 or 1.3.0), though. Ingame inventory/... rendering is even worse.
I could go on and on and on and on about this. See this or this (NOTE: I worked on this.) to get an idea of the hacks required to make modding possible, or grab a decompiler and let it run overnight to witness the horror yourself. (NOTE: I advise against all this if you still want to live a happy and carefree life.)
Sorry for giving you a braindump, but I need emotional relief. Working with this codebase gave me a PTSD of some sort.
(NOTE: I only had access to the decompiled source code, but MSIL->C# translation is quite accurate, so I can safely say that this applies to the original source code as well.)
Sorry.
Well I appreciate the insight, horrifying as it may be :D
Dear lord...
Oh hi @PoroCYon! I'm glad to know that you've stumbled on this too.
@raxod502 try and wrap your head around the decompiled version of Item.cs: https://raw.githubusercontent.com/Pryaxis/Sources/master/1.3.5.3/Terraria/Item.cs
Holy shit and here I was thinking it was just the compiler mangling everything.
And yet it's an extremely successful game that sold tens of millions of copies.
So there's a lesson in there. It doesn't matter if your code is "pretty" or "efficient". What matters is that it works and the game it works for is good.
Or maybe that's just wishful thinking of an awful programmer.
As far as I remember, Minecraft used to use the same magic number system for tiles and items. It meant that whenever someone used console commands to spawn an item, it was as if they had god memory. This has been replaced now, you can type tile names rather than ids in console, but I think that internally it still works the same way.
@UristMcDorf That's what you'd wish -- Terraria technically works, but it's a hell to work with, and it's extremely fragile (which shouldn't come as a surprise). It could've been coded in such a way that it'd run much faster (eg. because it currently allocates way too much every frame).
@hakusaro hi! o/
To everyone: here's a bonus:
- Tile IDs start from
0(dirt) and have a separateactiveflag, but walls are only defined by theirtype(0is none). - NPC and Projectile behaviour is determined by both the
type(sometimes even thenetID, but not always) of the entity, and itsaiStyle(yet another magic number). Both ways are defined -- for alltypes/netIDs -- in theAIorUpdatemethod, adding special cases everywhere. I remember someone cleaned upNPC.AIto have a good reference for theaiStyles and to use that code with some customisations in mods. I don't know if said person is still alive. - Sounds are defined by an
id, but some sounds (such as "item use" (1) and "NPC hit"/"NPC killed" (both somewhere between 5 and 10) have a sub-id. Sound playback was managed with one single method -Main.PlaySound(of course it's inMain), which manages sound instances, sets playback parameters (such as panning) and selects the rightSoundEffect(an XNA object) to play. TheseSoundEffects are not managed in an array, but for every ID, there is a separate field containing the effect (inMain, of course), and it is selected using... anif/elsechain. (The fields of sounds with sub-IDs are arrays instead of simple objects.) Instance management is done (looking very manually) for some sounds, but not for all, causing certain sounds not being able to be played back multiple times at the same time. (This had to be mimicekd usingugly hacksin modloaders. (Compared to what we're discussing, this is very neat code. Also, yes, some sounds are cut off and replayed when it can only be played once at the same time, while others cannot be started until the playing instance ends. This had to be mimicked as well...)) - Music is also managed in a single method --
Main.UpdateMusic. Fading is done manually (and for some reason exists twice in the method), and determining which track to play is done by a 300-lineif/else-chain soup. (I hope this doesn't really come as a surprise for anyone, but the ENTIRE CODEBASE is like this. It never ends.) - Prefixes are applied using the
Item.Prefixmethod. But, as it uses randomness to assign the exact values of the item stats, all the item properties need to be saved to disk. Chaos everywhere, especially when someone (dev or modder) forgot to do so in one place. - Redigit seems to be very afraid of the number
256: Most arrays that should be 256 elements long are actually 255 (eg.Main.player,Main.npc, and all the other arrays containing the entities in the world), and one (It was tile-related, iirc) had a length of257, but 256 was nowhere to be seen.
Now I want to read the comments in the original source code.
@Restioson Nope. Enums were used for tool types (wood, stone, iron, gold, diamond), but not for blocks or items. IIRC, there was one big static code block that defined all the block IDs. If you wanted to e.g. set a certain location in the world to be cobblestone (block ID 4), you would say world.setBlock(x, y, z, 4);.
Now I really want to read some insights from the original Terraria devs - sounds veeery interesting 🙂
I request this to be tagged as "wontfix"
https://www.reddit.com/r/ProgrammerDadJokes/comments/729qtl/comment/dnh9vds
Well now this repo is known on reddit for its messiness...
This is pure gold.
Reddit has rediscovered this work of art https://www.reddit.com/r/badcode/comments/cl2vxt/anyone_who_looks_at_this_code_instantly_becomes/
Hi,
I too work with Terraria modding, and I can confirm that the code is horrible. But I come from a different angle: I specialize in the multiplayer part of the game. I work on modded community servers and have worked extensively with the TShock framework. I can tell you a whole lot about Terraria's netcode protocol (guess what—it's bad, too):
The server application is simply a copy of the desktop game but with the rendering code taken out through conditional compilation symbols or something of the sort. Differences between server and client are differentiated with if chains using the format if(Main.NetMode == 2) and so on.
Packets are sent with a scrapped-together binary TCP protocol. You send a packet with the method NetMessage.SendData(short type, ...) and then a slew of payload arguments of various primitive types. Every packet is sent by this one method, and there is no override to it. Most payload arguments are never used, and often you'll have an int that's cast to a float and then cast back to an int again, just to match the arguments of the method.
Once you get down to the binary protocol, you basically have a 2-byte type argument and then a massive switch statement for the sender and receiver classes that parses the payload manually. Lots of undocumented reader.ReadInt32() and reader.ReadByte() to read through. If you said to me "packet 13," I'd know exactly what you were talking about from memory, and no man should know that
The concept of an "authoritative server" has no meaning in the Terraria codebase. When a player gets damaged, the client sends a PlayerDamage packet to the server, that then verifies the damage. I could go on. This, suffice to say, makes it a lot harder to combat cheating on large multiplayer servers.
Despite all this, I still choose to contribute to this community and actually enjoy doing it somewhat. This is possibly due to the fact that I huddle myself behind a fortress of wrapper classes to shield me from the particularly bad stuff, or possibly it's because I have Stockholm Syndrome. There's also a sort of sick appeal to making a functional multiplayer server with such strict limitations, kind of like how people write binary calculators in Super Mario Maker. There's some level of respect you have for those people, but you also feel a little bad for them.
It's the "UpdatePlayer" packet type.
or possibly it's because I have Stockholm Syndrome
Sshhh, that sounds too familiar to me.
Thankfully, I moved on... to somewhere where I code assembly all day.