Cryengine Source Code(github.com) |
Cryengine Source Code(github.com) |
EDIT: That whole function is a minefield. Just taking a quick look:
* 814 lines of code
* goto inside 3 nested for-loops
* macros
* commented out code
* new/delete, with no RAII
* thread specific variables and locks (?)
> * goto inside 3 nested for-loops.
C has no pattern for breaking out of multiple for loops at the same time. Other languages like Java and JavaScript introduced "break label;" to handle this edge case. goto is perfectly acceptable to break out of multiple loops.
> * new/delete, with no RAII
They use RAII, but it's not applicable here. In this case, the developers only want to allocate a temporary array when it needs to be resized. Since we don't want a large stack allocation (stack sizes are super small on consoles), using a delete/free to resize an array seems fine to me. Game developers have a long-seated distrust of std::vector, and for good reasons.
RAII is used for the WriteCondLock which you criticize in the next bullet point, so it's not like they were unaware of it. Just not the right tool for the job.
> * thread specific variables and locks (?)
You seem to be upset that they have code that uses locks at all? I don't really know what this bullet-point is saying, other than "I looked for 5 minutes and didn't understand the threading structure".
If I had to take an issue with this code, it's the lack of enums for e.g. iSimClass, despite it having an enum with definitions. That's the sort of stuff that's difficult to reason about and follow along with without having a mapping in my head. And it has no overhead, so why not do it?
https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d6...
Like all rules, there's always exceptions. In a hot code path like the physics engine logic, you're going for performance optimizations (which often LOOK messy) and that means making slight compromises on readability. It's quite a bit different than the code implementing the business logic in your SaaS company.
True, but it is cleaner to reorganize the code into several functions and use the return value to propagate across layers if needed. Performance should be the same.
> it's not applicable here
Why? If there is a new/delete pair anywhere, it should have been an object.
> Game developers have a long-seated distrust of std::vector, and for good reasons.
Which reasons? std::vector (and std::unique_ptr) is universally useful (unlike many other std data structures) and codegen should be on par as a new/delete pair.
If the std is completely broken in some of the console platforms they need to support (likely from what I hear here) then it can also be done with a custom heap array wrapper.
So I don’t see the reason why that shouldn’t have been an object.
Yeah, that's not what it does. The code looks like this:
if (pgeom->Intersect(pentlist[i]->m_parts[j].pPhysGeomProxy->pGeom, gwd,gwd+1, &ip, pcontacts)) {
got_unproj:
if (dirUnproj.len2()==0) dirUnproj = pcontacts[0].dir;
t = pcontacts[0].t; // lock should be released after reading t
return t;
}
for(int ipart=1;ipart<m_nParts;ipart++) if (m_parts[ipart].flagsCollider & pentlist[i]->m_parts[j].flags) {
gwd[2].R = Matrix33(qrot);
gwd[2].offset = pos + qrot*m_parts[ipart].pos;
gwd[2].scale = m_parts[ipart].scale;
gwd[2].v = -dirUnproj;
if (m_parts[ipart].pPhysGeomProxy->pGeom->Intersect(pentlist[i]->m_parts[j].pPhysGeomProxy->pGeom, gwd+2,gwd+1, &ip, pcontacts))
goto got_unproj;
}
}
EDIT: Just realised the poster is referring to a different function.
IMO this one is more horrifying.
Analysing code I'm not familiar with often consists of manually tracing through calls, producing documentation that inlines all the single use function calls.
Ideally function names act as shorthand for the body of the function, but if they only have one caller they have nothing to keep them honest. In older codebases, function names are as misleading as comments; semantic drift, special cases etc. mean you need to drill into them anyway.
In this contrived example, you can tell that formatting a title is not affected by user preferences, but formatting the body is. (And additionally that formatting a body has no information about the other fields of an entry)
def formatRssFeedEntries(userSettings: UserSettings, data: List[Entry]) {
val titles = data.map(entry => formatTitle(entry.title))
val bodies = data.map(entry => formatBody(entry.body, userSettings))
...
}Plus the Hungarian notation and whitespaces are not helping either.
When there is a test class to accompany this thing it would maybe help to faster make sense of it. But that‘s still no fun.
Splitting it up and using functions to extract use cases would help tremdendiously.
This code is clearly not perfect, but the from what I've seen, this is something I could work with.
- Function names are easy to read and understand. - Indirections are kept to a manageable level.
I came across the same kind of thing when I was kicking the tires on the Unreal Engine, and I wanted to attempt to add a double jump. I thought surely this should be an easy task, I would just need to find where the jump occurs, add a counter, and remove the restriction which only lets a character jump when touching the ground. What I found was a monstrous tangle of indirection similar to this one.
Now that's not to say that these engines are "bad code" - when you look at all the things a modern game engine does, including supporting interactive editing for non-coders, I'm sure there is some explanation for the level of complexity seen in code like this just because of how many systems must be layered on top of each-other. But that is the thing which makes me question whether general-purpose game engines are really a good idea at all.
In most other domains of software we've long ago eschewed this type of do-everything monolithic software design in favor of more loosely coupled composible toolsets. I'm not entirely sure why it seems that game development has yet to escape this paradigm.
At least the logic is all there and you only need to scroll to see it, instead of jumping around between a dozen or more different files.
Math-heavy code tends to look dense to those who are accustomed to more "mundane" LOB type applications.
However, can you imagine how bad an enterprise C++ project would look like?
It's actually a reasonable practice in C and C++ to use goto to leave nested loops (I am hesitating to say a recommended practice).
There is often no sane way to leave the loop otherwise, break/continue statements only have effect in the most inner loop. It's possible to set extra variables with lots of if/break but that gets crazy real quick and much slower (nested loops are often the hot code path).
This is the worst code to read in the engine by far. At the end of the day though it worked, we shipped it and it performed well enough.
Last I checked Lumberyard still has a somewhat cleaned up version of this function.
most of your points are things people notice any time real actual big software with performance constraints gets posted. so along with questioning the wisdoms in that function, also question your own wisdom. maybe some of what you think you know is wrong.
> //FIXME: There's a threading issue in CryPhysics with ARM's weak memory ordering.
https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d6...
Translation: "We have race conditions in our C++, but x86 is lenient enough and current MSVC not aggressive enough to make it crash and burn constantly on our main platform."
Coincidentally, I finished Crysis 1 today. That involved the first four crashes I had with the game, three of which were in the final battle.
https://stackoverflow.com/questions/72275/when-should-the-vo...
* magic numbers
m_iSimClass = 3;
return 1E10;
m_timeSmooth*(1/0.3f)
sqr(0.0001f)
m_pos.len2()>1E18
helper.pos.len2()<50.0f
sqr(m_size.x)*g_PI*0.25f && maxdim<m_size.z*1.4f)
*1.05f
*0.4f
*0.2f
<0.087f
When I was getting my CS degree, my professors required, at the very least, to describe what each method does, what each argument is, and a possible range of values of each, and the same for the return value.
I hate this "self-documenting" nonsense, which obviously doesn't work. This piece of code is a good example of that.
Ultimately, we're trying to simplify a large simulation of real-world physics and hundreds of controllable muscles and motor responses trained over a lifetime, down to 5 or 6 keyboard inputs. That means you tend to have such inputs doing multiple things, and it's often hard to make separable.
I feel like this Cryengine example may be a bad example of that, though.
So function extraction ends up taking a bunch of "unrelated" state with it where, in the end, it feels like you accomplished nothing but split the code into arbitrary concatenation points, not logical units.
Software that a lot of people use a lot of time looks like this because the bugs are found and the bugs get fixed. The code for fixing a bug has to live somewhere.
What about bugs that computers find? Fuzzers are rarely recommending to fix bugs that are affecting human users. Part of that is also that the kinds of bugs that computers can find are not in, literally, "user interfaces," they are in APIs and formats.
Anyway, end user business software also has code that looks like this. It's not just all tools and infrastructure, I mean it certainly feels that way. But there are 800+ line SQL statements. 800+ line transactional method bodies. I don't want to call this "real code" but the surprise comes from... well eventually you have to make something the end user touches. And it's going to be gnarly.
For a classic example, it's makes almost no sense separate model and view when you're building a real time action game, because your rendering code and your ballistics and physics work on very similar 3d meshes that are animated by the same skeleton animations and tied to the same objects.
There is also a lot of fear of performance regressions by breaking up big chunks of code (arguably unfounded with modern compilers).
There is nothing wrong with this. Plenty of people have no issues keeping track of memory in their head.
Even ignoring the “We may change this licence at any time and it applies to you” parts, there seem to be serious restriction on usage and basically have to ask them to do _anything_ beforehand.
Maybe it makes sense if you are already in a project that is using this Licenced? Is this intended as a general engine licence rather than viewing the source code?
I suspect that lumberyards greatest advantage over cryengine in the future will simply be usable documentation provided by amazon. Cryengine is simply not usable without better docs or else an incredible amount of time. Crytek is having financial troubles but I bet their engine would have 10x adoption if they hired a team technical writers
Unreals docs are fairly bad also, but at least there are some third party resources to turn to
Case in point: Vehicle physics is no where near as good as the docs imply (not a toy but still 20 year old vintage), but there is almost no documentation of how PhysX interacts with the Unreal engine proper i.e. you can get the PxRigidWhatever handle but you can't easily replace PhysX with a proper MB package. Epic seem to be transitioning to Chaos but it's not documented yet.
If I ever get good Vehicle physics working I'll write it up (it's definitely possible but I'm not sure how ACC does it)
The consequences of taint are a very real risk here.
Related significant threads: https://hn.algolia.com/?dateRange=all&page=0&prefix=true&que...
master (now main) was not always stable (of course, stable code are in the stable and release branches) so silly people complained, and the silly PM reacted by closing down pushes to main, and hereby closing down issues and PR's. He clearly has no idea how open source code development works. Now they have to maintain two repos, the internal one and thd public one, and get no feedback from outside. Well, feedback on one year old code.
Many very highly performance tuned applications I saw in the wild would fall into the category of "horrible codebase" when looked at through that lens.
I understand that you might think that messy could mean it's fine tuned for performance. In this case, I highly doubt it and think it's more reasonable to think it's messy because they had deadlines.
The messy part isn't about performance optimizations. It's more about things that got crammed in there and only works for a very specific subset of parameters. And even then you can't be sure it'll work...
I don't blame the programmers, it feels they had deadlines to uphold from managment.
If we want the code to be clean or performant, we will likely have to spend time iterating on and pruning the code. Let’s assume that improving performance and improving cleanliness are at best orthogonal, at worst opposing.
The project has a limited amount of time, particularly for games, which often have a relatively low roof for how much maintenance the code will need.
The project has a budget on time to spend between cleanliness and maintenance. Games need high performance and relatively little maintenance, so they are more likely to spend their budget on much more performance than cleanliness.
(Game engines meant for heavy reuse such as Frostbite and Unreal Engine would likely have a much more even split, and similar for games which are likely to receive recurring and invasive updates. I would expect Fortnite’s code to be fairly clean as games go, for example.)
The issue with games in particular is probably partly due to the performance optimizations being directed at a moving target (it's not just your supercomputer nodes, it's every computer CPU). C++ doesn't really help you much in that regard (or at least better know but certainly not 10 years ago)
Open source generally leads to better quality code, since it's the best way to attract other developers to contribute to it.
So I'm rarely interested by any accomplished project that opens its code.
For example, if microsoft opened its OS, I doubt developers would really try to do things with it. The windows kernel would obviously be high quality code, but a lot of the rest is probably short lived garbage.
I think you got lost in your triple negative there
I want to make the basics of an open source (sim)racing game (as a test bed for writing tyre models), without using the fairly lacklustre offerings included by default (e.g. no sprung mass, no carcass stiffness etc., idealized suspension etc.). I have no need to go into the bowels of the rendering engine but it struck me that the interactions between PhysX and the actual actor model for the vehicle is almost not documented at all. I assume it's possible to do it solely with PhysX (proper suspension) but I cannot find any case studies of people doing it with the possible exception of their drive project which is under $$$ and NDA I'm guessing.
I was also slightly surprised that I had to go looking for the option to connect to the PhysX debugger. It wasn't hard to find but I was half expecting it to be included with the engine.
Just as an example, this is why Bitwarden started getting some automated testing - lots of propelled bumping github issues about it in order to get it more visibility
There's terrible code all over the place, although it is definitely true that no one's going to clean up - even source available - proprietary code out of kindness of their heart.
[0] https://www.polygon.com/2020/1/13/21064100/vvvvvv-source-cod...
It's a bit of a strange thing; in my personal experience, after spending some real time with this type of constraints, it can be a bit painful to come back to "general software best practices". You become so aware of the performance implications of everything you do that all those things we do in the name of software quality can feel incredibly wasteful in terms of CPU and memory resources. One has to remind themselves that in 90% of cases that level of optimization is not warranted.
Placing a floor that has the influences you want under the player is, in a sense, a hack, but not an entirely misguided one. A better design might be not relying on whatever the "jump" button does, but writing your own, e.g. applying an impulse to the player directly.
If this sounds like semantics, consider a game like Super Mario Galaxy, which has spherical worlds. How do you determine the correct impulse for which way a "jump" goes? This is where these sorts of complications come from.
Old players maybe remember that the crysis multiplayer was the most cheated game in its era. It was totally unplayable due to all the cheating and that killed the game.
One way to make cheats. You could load up the SDK in visual studio. Find the code that's removing -1 ammo when shooting and edit it to not do that (most of the physics and game logic was editable that way). Compile the DLL. Replace the original DLL in the game directory.
Good game though, I think Crysis 1 multiplayer is to this day one of the most fun multiplayer games I have played. After Crysis 1 they really CoD'fied the game, which is a shame. That and BF2142, A-plus games.
Looking forward to Crysis Remastered, hope they don't screw it up.
UE4 has a strong bias about the way things should work. If I am making something which is fairly well aligned to that bias, then it's fairly easy to make it work. But if I want to achieve something which is quite far from what the engine expects, then I have to invest significant effort undoing or circumventing what UE4 already does before adding my own functionality on top. I would greatly prefer to start from a blank slate, and only add precisely the behavior I actually want.
So basically this experience with the double jump just gave me a window into the level of complexity I would have to work around in terms of realizing my own goals.
Games like Valorant are using their own modified version, and from their own admission, every version bump must be carefully considered and takes upwards of a month to reintegrate their changes in a painstakingly slow process.
This is my own experience. The teams that spent the most time on standards usually had the least pressure, in terms of things like deadlines. Once the focus of the team shifts to having to ship things, there is less time to worry about having 100% code coverage (to pull out an arbitrary number), and so forth. Code review can slip into flagging only things that really matter, and leaving nitpicks for another day.
https://www.viva64.com/en/b/0417/
https://www.viva64.com/en/b/0495/
https://www.viva64.com/en/b/0574/
I'm not endorsing pvs studio nor am I saying it's bad. Try out some tools and see what works best for you.
I disagree. Having to jump to another function definition which is inline is a bigger mental block than following a goto. The large amount of arguments you'd need to pass might also be a barrier, as is the mental overhead of checking to see if this function might be called from elsewhere. But reasonable people can disagree on this point.
I suggest you try to clean up the function yourself and see if your function-ed version is in any way improved.
> Why? If there is a new/delete pair anywhere, it should have been an object.
The case we want is that we have a large array of pairwise collisions. This is a temporary array used inside the method. If we ever have more than this, we need a new (contiguous) array. Are you suggesting that the code should have done something like this?
template<typename T> class TempArray { T* storage; void resize(int n) { delete[] storage; storage = new T[n]; };
I mean, sure, it's a very minor cleanup. It changes like, two lines though, and makes us have to access the array through an awkward ->storage pointer. Not ideal IMO.
> codegen should be on par as a new/delete pair.
Here's modern MSVC. Let's play "spot the difference": https://gcc.godbolt.org/z/KqR-LN
I'm not even doing anything but allocating the vector / array. It took me 2 minutes to navigate to godbolt and type this in. Don't just say "should be"... test it yourself!
It is a good illustration of why debug stl builds are such hot garbage though...
Local lambdas are ideal for this.
> Are you suggesting that the code should have done something like this?
Yes, but you can manage the array inside too.
> I mean, sure, it's a very minor cleanup. It changes like, two lines though
The point is that TempArray can be reused everywhere. This is a typical class that many projects use (stack if small, heap is bigger than threshold).
> Here's modern MSVC. Let's play "spot the difference"
The optimizer has been asked to leave everything as it is, so that is the expected result.
BTW, MSVC is not what you should be using if you want performance.
> Don't just say "should be"... test it yourself!
I always test codegen for all abstractions I use! So I agree.
The problem with lambdas, you can't mark their operator() with __forceinline or __attribute__((always_inline)) attributes. For this reason, when writing high-performance manually vectorized code, lambdas are borderline useless.
> MSVC is not what you should be using if you want performance.
Security and compatibility has higher priority. gcc and clang don't deliver their C runtime libraries with windows updates. Also, debugging and crash diagnostic is much easier with MSVC.
It's same on Linux BTW, only with gcc.
As a side note, you're not calling delete[] on the array code, but I suppose it makes little difference if you take the vector cleanup into account.
Jasper_ is the real deal, people.
Those blanket statements like "never use goto" and "always trust stl" generally make me wary. I started out with gwbasic once, writing horrible goto spaghetti code. When moving through Pascal and C I eventually learned the "never use goto" mantra and naively tried to follow it at all cost. After I while I eventually encountered the "breaking out of nested loops" problem and refrained from using goto since I didn't want to look like a complete dork. I think I ended up with a flag that was set in the inner loop and checked in the outer and a break in each loop. That's what you get from blindly following rules that others try to present you as god-given.
It also makes no sense to have a rule on the maximum number of lines for a function. These rules are often created by people who don't write software that needs to have high performance.
How common is automated testing in AAA games?
The point was not about manually vectorized loops in particular. Why is that a problem if you are manually doing it, though?
> Security and compatibility has higher priority.
In commercial games, not really.
As for "compatibility", I am not sure what you mean.
> gcc and clang don't deliver their C runtime libraries with windows updates.
AFAIK you can use Windows libraries just fine. No need for using a different libc.
> Also, debugging and crash diagnostic is much easier with MSVC.
AFAIK, Clang can produce debugging info that you can use with VS.
I don't work on the environment, but it is what I have read here.
Here’s couple examples: https://github.com/Const-me/DtsDecoder/blob/master/Utils/App... https://github.com/Const-me/SimdIntroArticle/blob/master/Flo... I would like to use lambdas instead of classes, but can’t, due to that defect of C++.
> In commercial games, not really.
In commercial games too. While they don’t care about security, they do care about compatibility and crash report diagnostics.
> As for "compatibility", I am not sure what you mean.
A windows update shouldn’t break stuff. A software or a game should run on a Windows released at least 10 years in the future.
> Clang can produce debugging info that you can use with VS.
According to marketing announcements. This page however https://clang.llvm.org/docs/MSVCCompatibility.html only says “mostly complete” and also “Work to teach lld about CodeView and PDBs is ongoing”. PDB support is not about VC compatibility, it’s about Windows compatibility really: the debugger engine is a component of OS, even of the OS kernel. WinDbg is merely a GUI client for that engine, visual studio is another one.
Overall, in my experience, the platform-default compilers cause the least amount of issues. On Windows this means msvc, on Linux gcc, on OSX clang. Technically gcc and clang are very portable. Practically, when you’re using a non-default toolset, you’re a minority of users of that toolset, e.g. the bugs you’ll find will be de-prioritized.
How would you implement something simple like a lookup in an array? From this argument even a break in a normal for-loop would be bad since I wouldn't know anymore how many iterations it will take. So if you have a nested loop and a "goto end_outer_loop" that's perfectly fine.
The most famous example I can think of was UOExtreme revealing hidden players, because if a player was hiding their presence was still sent to the clients of every other player in the area just with a "hidden" flag set. There were a bunch of other similar exploits associated with that particular third-party tool, but that's the only one I remember.
(so while writing this I did some googling and found the patch notes where they fixed it: https://uo.com/wiki/ultima-online-wiki/technical/previous-pu...)
- The function is exported to a library
- The code generator emitted an indirect branch instruction
- The inlined code exceeds the size of a single page in memory
- The inlined code does not perform as well because it is not i-cacheable
This rationalization doesn’t make sense to me, especially for game engines which I assume are not limited to a release or 2 but are used to power multiple titles. If the software artifact is going to live for a long time, it’s probably worth the effort to make it easier to understand and test.
Could this pattern result in code so slow the game won't work in development w/o high optimization flags, thus increasing compile time?
There may be other criteria, eg is the function likely to be called.
That is objectively false. Even though I agree with the spirit. For starters, unoptimized builds won't have that. Second, exceptions tend to inhibit inlining. Compare https://godbolt.org/z/c8Jayf with https://godbolt.org/z/Uoo2YA. Third, it's easy enough to push one of the heuristics used for inlining high enough in the wrong direction (e.g. function size). I recommend the clang optimizer view on godbolt.
Also note that it can be hard for a compiler to prove that a function is not used outside the current translation unit (although anonymous namespaces help with that). Number of calls is only one of the heuristics for inlining though anyway.
Compilers are plenty stupid, and have not earned blind trust.
Games that use automated tests often drive high level systems and test high level output. See for instance Riot Games's automated League of Legends test suite.
Almost everything taught in academia and in the enterprise about software development "best practices" are absolutely the wrong things for a game. (They're wrong for enterprise and academia, too, but I'm not willing to get into that fight on this site.)
The power of an engine is that it is a series of closely intertwined parts. Often times, the graphics code and collision code are linked; here's an example. Shoot a bullet in any FPS; the resulting decal is likely created from the collision data, since it's much faster to search than the highly tessellated graphics model.
Or, let's say, I have a melee attack. The specific frame that the attack occurs on is likely decided by the animation data. So that means that we need to run animations on the server, against an invisible skeleton.
Similarly, you might encounter a scenario where a bunch of stuff blows up, and you want 99% of that simulation to be baked offline and not simulated on the client, to ensure that the tower lands exactly where it needs to. So now you need to plug together your physics and animation systems, yadda yadda.
These are not bugs, or necessarily problems. People who suggest we turn all engines into tiny independent toolkits are missing the point. Of course everyone tries to make systems as independent as possible. But there's a bunch of power you can unlock if you have even a small bit of close integration between two systems.
How does one manage complexity? The first is to enable everyone, even artist and designers, to be technical. They're already managing complex topologies, doing sophisticated lighting and shading techniques, and often writing custom Python scripts to help them clean up the massive amount of data. A big thing I've seen from developers out of FAANGs is that they're shocked that "non-programmers" are writing custom scripts and doing programming. Your artists and designers are incredibly technical people who are good at solving problems. Trust them.
Invest in custom tools; any bit of custom tooling to make your life easier will pay off massively. Basically, give your whole staff the ability to debug and fix issues, with custom UIs and logs specifically built for your purpose.
QA staff are undervalued here in the US, but anybody who's worked in games for three months knows what a difference a senior QA makes. They know your game better than anybody else. Watching a good QA go from a blurry cameraphone pic on an angry reddit post somewhere to complete repro steps in less than an hour is nothing short of magic.