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;
}
}
Notice (a) the if statement on the same line as the for loop, and (b) the fact that the goto jumps out of a conditional inside a for loop inside of a conditional into a conditional just before the for loop.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
* Very few comments explaining what's going onWhen 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)
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
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.