13 ms·

This is on the front page again. Please take some time to read this wonderful function: https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEngine/CryPhysi

by Nican 6y ago

This is on the front page again. Please take some time to read this wonderful function: https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEngine/CryPhysics/livingentity.cpp#L1300 https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEng... 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 (?)

deleted 6y ago
[deleted]
gentleman11 6y ago
800 lines long. The movement component class in ue4 is about 10k lines. Why do game engines separate their code so much less than in other software?
- Trasmatta 6y ago
  Here's a good article from John Carmack on why that can be a good approach in game development: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm... I feel like this Cryengine example may be a bad example of that, though.
  - deleted 6y ago
    [deleted]
  - smaddox 6y ago
    Not just in game development. If you have a function that is only called once, it shouldn't be a function yet. Make it a function when you have a second or third use for it. Then, and only then, you will know what the parameters should be.
    - oever 6y ago
      I disagree. Splitting a function up can help with readability and testability. The parent function becomes shorter and the child function can have a descriptive name. The parameters to a function are the fields that are needed for the function to perform its function.
      - gentleman11 6y ago
        One of my professors used to encourage the heavy use of helper functions that just... well, like in A or B in carmacks article, break your code into chunks with clear names that can be unit tested. How common is automated testing in AAA games?
        Jasper_ 6y ago
        Underlying helper libraries like math utilities are often extracted and put under independent test. A physics engine often has so much state (and isn't always guaranteed to be deterministic!) that doing any sort of unit testing at the functional level is not worth it. To those that reply with "use less state", I encourage you to show how. Often times the unit tests I've seen from junior game programmers are worse than useless and aren't testing anything of tremendous value. 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.
        naikrovek 6y ago
        Game development problems are often large global state manipulation problems. If you don't write games you will never realize this. 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.)
        gentleman11 6y ago
        Is there an industry-wide set of concepts and best practices that is applicable for large scale games? What sorts of things do hiring managers worry about at night?
- hombre_fatal 6y ago
  Sometimes with inherently complex performance code it gets nickel and dimed over time yet maintains so many cross-cutting concerns and state that there are no clean extraction points. 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.
- an_opabinia 6y ago
  Games are certainly the most popular piece of end user software. 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.
- mhh__ 6y ago
  I feel like C++ makes it particularly difficult to jump around big projects (mainly headers especially back in the old days pre good-ide). There is also a lot of fear of performance regressions by breaking up big chunks of code (arguably unfounded with modern compilers).
- golergka 6y ago
  Apart from performance considerations, it's also because in game development, different systems are much more entangled together in the requirements themselves, in many very small and unpredictable ways that still break architectural boundaries you put in. 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.
ajconway 6y ago
Is it an example of bad code? Should we avoid using Cryengine?
- krapp 6y ago
  If it works, it's an example of ugly code. Plenty of game code is ugly as sin, though.
  - peterkos 6y ago
    The code for VVVVVV infamously has switch/case for every possible screen/level in the game[0], which I think is hilarious. Best example of "it works!" [0] https://www.polygon.com/2020/1/13/21064100/vvvvvv-source-code-game-development-terry-cavanagh-release https://www.polygon.com/2020/1/13/21064100/vvvvvv-source-cod...
    - skohan 6y ago
      Games also have a very different set of constraints than most software. Simulating and rendering an interactive world at 60-144hz is not an easy task, and things like code cleanliness, structure and readability often lose out to performance concerns. 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.
stephc_int13 6y ago
As a professional game developer I disagree with most of your comments. 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.
- Nican 6y ago
  Not going to lie, playing Crysis was a lot of fun, and I never knew this was the underneath function running it.
  - user5994461 6y ago
    Crysis shipped with a full blow SDK that included most of its source code. You could actually rebuild the game from it, the 50MB dll that controlled the whole game. 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.
    - Nextgrid 6y ago
      Was ammo in Crysis controlled client-side? I've always assumed such counters were stored server-side and thus a server won't apply a "fire" event if the player's ammo counter is at zero until it receives a "reload" event to reset the counter.
      - numpad0 6y ago
        One explanation I’ve seen about weak server-side verification is online multiplayer is a cost center so developers wants to offload much as they could. At least before microtransactions I guess.
      - user5994461 6y ago
dmitrygr 6y ago
> new/delete, with no RAII There is nothing wrong with this. Plenty of people have no issues keeping track of memory in their head.
- skohan 6y ago
  I would argue that manual memory management should probably be avoided as a general best practice, but game development is a special case where memory management can often be critical to the performance of the final product, and a manual approach is sometimes warranted.
barrkel 6y ago
If the code was separated out into functions that are only ever called once, I'd find it harder to read. 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.
- mopierotti 6y ago
  I agree with the gist of your post, but one positive about single use functions is that you can be explicitly clear about data visibility. 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)) ... }
  - barrkel 6y ago
    I agree. Blocks can somewhat substitute with scoping effects, and that's what I do with my inlined docs - they're nested {} with plain text description of contents and mentions of key variables and functions, scope is useful.
- myspy 6y ago
  Nested ifs in for loops, with lots of things going on here, I don‘t know but that‘s the definition of hard to maintain and to read code. 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.
rurban 6y ago
This is physics, not your average function. Those who've never written physics should not complain. It's actually good code.
atombender 6y ago
The code looks bad, but not for those reasons, in my opinion. The logic in this function doesn't look composable. It combines different kinds of mathematical functions to apply inertia, whether you're jumping, etc. into one big ball of spaghetti that would be hard to extend for anyone not deeply familiar with the code. If I were to try to refactor this, I would try to decompose it into standalone "behaviour" functions that could be attached to any entity
- Jasper_ 6y ago
  I encourage you to try! However, a lot of gameplay and getting movement controls to feel good is difficult and at odds with the goal of "modularized physics", e.g. you might want to apply different amounts of ground friction depending on whether the player is moving, how they're moving, whether they're holding the jump key, and so on. 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.
  - gmueckl 6y ago
    I guess, a sufficiently simple exercise would be to write a controller that handles ground movement, jumping with air movement on horizontal ground and moving platforms. Doesn't have to feel good to play, just be reasonably robust. Either handling of slopes and stairs or collisions with dynamic objects can be added for extra credits ;).
skohan 6y ago
As a hobbyist who likes to tinker with game and interactive media development, a sentiment I often come across is that in 2020 it makes no sense to implement a game engine, and that I should just use something which already exists to avoid re-inventing the wheel. Code like this is one thing which helps me to calmly ignore than sentiment. 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.
- rtx 6y ago
  We haven't at place where deciplines interact.
- christoph 6y ago
  I have a feeling you may have been approaching this problem in UE4 the wrong way. Adding a double jump can be done in numerous ways, but one simple way is with Blueprints. See below link where the exact functionality is implemented with a really simple blueprint. https://m.youtube.com/watch?v=hFAr7gYV1rA https://m.youtube.com/watch?v=hFAr7gYV1rA
  - skohan 6y ago
    Oh I am certain I was not approaching the problem in the UE4 way. But the issue is that the way UE4 expects me to do things is not the way I would like to approach game development. 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.
Jasper_ 6y ago
I don't think it's the best code I've ever seen, but a lot of these are surface-level complaints. > * 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/6c4f4df4a7a092300d630f8f89d2ebda39183c36/Code/CryEngine/CryCommon/CryPhysics/physinterface.h#L121 https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d6...
- messe 6y ago
  > 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. 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.
gameswithgo 6y ago
inside a bunch of nested loops is where you need a goto, if the language doesn’t have labeled breaks (which is just a fancy goto) 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.
- qeternity 6y ago
  Code like this actually always makes me feel better about my own code when I’ve ultimately had to make a trade off between abstraction/organization and performance. I wonder who all these engineers are that see code like this, especially in hot paths, and can’t understand how it came to be and that there was a deliberate choice made.
loeg 6y ago
There's some RAII: https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEngine/CryPhysics/livingentity.cpp#L2054 https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEng...
user5994461 6y ago
* goto inside 3 nested for-loops 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).
aronpye 6y ago
It’s not just that function, the C file is over 2300 lines long. It’s hard to tell where one function starts and another one ends in that mess
- kccqzy 6y ago
  Why not? Doesn't the indentation tell you perfectly where each functions starts and ends?
nolaspring 6y ago
Also Requires visual studio
layoutIfNeeded 6y ago
"You may not like it, but this is what peak performance looks like."
userbinator 6y ago
I'd rather work with this than Enterprise Java. 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.
- kccqzy 6y ago
  This. The convenience of everything in a single file is underrated. You can do all the modern best practice like splitting into more functions, making functions small, and I wouldn't mind if they are all in the same file.
- bitcharmer 6y ago
  Being a Java dev (not enterprise any more) I agree that some enterprise software is an abomination for exactly the reason you mention. However, can you imagine how bad an enterprise C++ project would look like?
  - rowanG077 6y ago
    A gameengine is as enterprise as it gets.
bufferoverflow 6y ago
And * 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 on 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.
- isatty 6y ago
  Not defending the use of magic numbers but some of it is clear for someone who has written physics code before.
MauranKilom 6y ago
This is what draws my attention more: > //FIXME: There's a threading issue in CryPhysics with ARM's weak memory ordering. https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d630f8f89d2ebda39183c36/Code/CryEngine/CryCommon/CryPhysics/physinterface.h#L133 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.
- 361994752 6y ago
  This is not about race condition. Rather it is something more like why you need volatile keyword. https://stackoverflow.com/questions/72275/when-should-the-volatile-keyword-be-used-in-c https://stackoverflow.com/questions/72275/when-should-the-vo...
  - MauranKilom 6y ago
    Can you elaborate? volatile and threading in C++ are orthogonal to each other. I don't understand why you consider a C# volatile discussion relevant when talking about C++ race conditions.
  - ensiferum 6y ago
    Volatile in c++ has nothing to do with threading.
wesmac 6y ago
I knew which function this was before even clicking the link. I spent months working on and debugging that exact function and the surrounding CryPhysics code improving network interpolation back in 2014. All the undefined behavior caused us some trouble while porting to the "next-gen" consoles of the time. 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.

by Nican 6y ago

deleted 6y ago

[deleted]

gentleman11 6y ago

800 lines long. The movement component class in ue4 is about 10k lines. Why do game engines separate their code so much less than in other software?

Trasmatta 6y ago
Here's a good article from John Carmack on why that can be a good approach in game development: http://number-none.com/blow/john_carmack_on_inlined_code.html http://number-none.com/blow/john_carmack_on_inlined_code.htm... I feel like this Cryengine example may be a bad example of that, though.
- deleted 6y ago
  [deleted]
- smaddox 6y ago
  Not just in game development. If you have a function that is only called once, it shouldn't be a function yet. Make it a function when you have a second or third use for it. Then, and only then, you will know what the parameters should be.
  - oever 6y ago
    I disagree. Splitting a function up can help with readability and testability. The parent function becomes shorter and the child function can have a descriptive name. The parameters to a function are the fields that are needed for the function to perform its function.
    - gentleman11 6y ago
      One of my professors used to encourage the heavy use of helper functions that just... well, like in A or B in carmacks article, break your code into chunks with clear names that can be unit tested. How common is automated testing in AAA games?
      - Jasper_ 6y ago
        Underlying helper libraries like math utilities are often extracted and put under independent test. A physics engine often has so much state (and isn't always guaranteed to be deterministic!) that doing any sort of unit testing at the functional level is not worth it. To those that reply with "use less state", I encourage you to show how. Often times the unit tests I've seen from junior game programmers are worse than useless and aren't testing anything of tremendous value. 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.
        naikrovek 6y ago
        Game development problems are often large global state manipulation problems. If you don't write games you will never realize this. 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.)
        gentleman11 6y ago
        Is there an industry-wide set of concepts and best practices that is applicable for large scale games? What sorts of things do hiring managers worry about at night?
hombre_fatal 6y ago
Sometimes with inherently complex performance code it gets nickel and dimed over time yet maintains so many cross-cutting concerns and state that there are no clean extraction points. 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.
an_opabinia 6y ago
Games are certainly the most popular piece of end user software. 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.
mhh__ 6y ago
I feel like C++ makes it particularly difficult to jump around big projects (mainly headers especially back in the old days pre good-ide). There is also a lot of fear of performance regressions by breaking up big chunks of code (arguably unfounded with modern compilers).
golergka 6y ago
Apart from performance considerations, it's also because in game development, different systems are much more entangled together in the requirements themselves, in many very small and unpredictable ways that still break architectural boundaries you put in. 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.

ajconway 6y ago

Is it an example of bad code? Should we avoid using Cryengine?

krapp 6y ago
If it works, it's an example of ugly code. Plenty of game code is ugly as sin, though.
- peterkos 6y ago
  The code for VVVVVV infamously has switch/case for every possible screen/level in the game[0], which I think is hilarious. Best example of "it works!" [0] https://www.polygon.com/2020/1/13/21064100/vvvvvv-source-code-game-development-terry-cavanagh-release https://www.polygon.com/2020/1/13/21064100/vvvvvv-source-cod...
  - skohan 6y ago
    Games also have a very different set of constraints than most software. Simulating and rendering an interactive world at 60-144hz is not an easy task, and things like code cleanliness, structure and readability often lose out to performance concerns. 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.

stephc_int13 6y ago

As a professional game developer I disagree with most of your comments. 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.

Nican 6y ago
Not going to lie, playing Crysis was a lot of fun, and I never knew this was the underneath function running it.
- user5994461 6y ago
  Crysis shipped with a full blow SDK that included most of its source code. You could actually rebuild the game from it, the 50MB dll that controlled the whole game. 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.
  - Nextgrid 6y ago
    Was ammo in Crysis controlled client-side? I've always assumed such counters were stored server-side and thus a server won't apply a "fire" event if the player's ammo counter is at zero until it receives a "reload" event to reset the counter.
    - numpad0 6y ago
      One explanation I’ve seen about weak server-side verification is online multiplayer is a cost center so developers wants to offload much as they could. At least before microtransactions I guess.
    - user5994461 6y ago

dmitrygr 6y ago

> new/delete, with no RAII There is nothing wrong with this. Plenty of people have no issues keeping track of memory in their head.

skohan 6y ago
I would argue that manual memory management should probably be avoided as a general best practice, but game development is a special case where memory management can often be critical to the performance of the final product, and a manual approach is sometimes warranted.

barrkel 6y ago

If the code was separated out into functions that are only ever called once, I'd find it harder to read. 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.

mopierotti 6y ago
I agree with the gist of your post, but one positive about single use functions is that you can be explicitly clear about data visibility. 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)) ... }
- barrkel 6y ago
  I agree. Blocks can somewhat substitute with scoping effects, and that's what I do with my inlined docs - they're nested {} with plain text description of contents and mentions of key variables and functions, scope is useful.
myspy 6y ago
Nested ifs in for loops, with lots of things going on here, I don‘t know but that‘s the definition of hard to maintain and to read code. 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.

rurban 6y ago

This is physics, not your average function. Those who've never written physics should not complain. It's actually good code.

atombender 6y ago

The code looks bad, but not for those reasons, in my opinion. The logic in this function doesn't look composable. It combines different kinds of mathematical functions to apply inertia, whether you're jumping, etc. into one big ball of spaghetti that would be hard to extend for anyone not deeply familiar with the code. If I were to try to refactor this, I would try to decompose it into standalone "behaviour" functions that could be attached to any entity

Jasper_ 6y ago
I encourage you to try! However, a lot of gameplay and getting movement controls to feel good is difficult and at odds with the goal of "modularized physics", e.g. you might want to apply different amounts of ground friction depending on whether the player is moving, how they're moving, whether they're holding the jump key, and so on. 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.
- gmueckl 6y ago
  I guess, a sufficiently simple exercise would be to write a controller that handles ground movement, jumping with air movement on horizontal ground and moving platforms. Doesn't have to feel good to play, just be reasonably robust. Either handling of slopes and stairs or collisions with dynamic objects can be added for extra credits ;).

skohan 6y ago

As a hobbyist who likes to tinker with game and interactive media development, a sentiment I often come across is that in 2020 it makes no sense to implement a game engine, and that I should just use something which already exists to avoid re-inventing the wheel. Code like this is one thing which helps me to calmly ignore than sentiment. 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.

rtx 6y ago
We haven't at place where deciplines interact.
christoph 6y ago
I have a feeling you may have been approaching this problem in UE4 the wrong way. Adding a double jump can be done in numerous ways, but one simple way is with Blueprints. See below link where the exact functionality is implemented with a really simple blueprint. https://m.youtube.com/watch?v=hFAr7gYV1rA https://m.youtube.com/watch?v=hFAr7gYV1rA
- skohan 6y ago
  Oh I am certain I was not approaching the problem in the UE4 way. But the issue is that the way UE4 expects me to do things is not the way I would like to approach game development. 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.

Jasper_ 6y ago

I don't think it's the best code I've ever seen, but a lot of these are surface-level complaints. > * 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/6c4f4df4a7a092300d630f8f89d2ebda39183c36/Code/CryEngine/CryCommon/CryPhysics/physinterface.h#L121 https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d6...

messe 6y ago
> 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. 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.

gameswithgo 6y ago

inside a bunch of nested loops is where you need a goto, if the language doesn’t have labeled breaks (which is just a fancy goto) 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.

qeternity 6y ago
Code like this actually always makes me feel better about my own code when I’ve ultimately had to make a trade off between abstraction/organization and performance. I wonder who all these engineers are that see code like this, especially in hot paths, and can’t understand how it came to be and that there was a deliberate choice made.

loeg 6y ago

There's some RAII: https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEngine/CryPhysics/livingentity.cpp#L2054 https://github.com/CRYTEK/CRYENGINE/blob/release/Code/CryEng...

user5994461 6y ago

* goto inside 3 nested for-loops 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).

aronpye 6y ago

It’s not just that function, the C file is over 2300 lines long. It’s hard to tell where one function starts and another one ends in that mess

kccqzy 6y ago
Why not? Doesn't the indentation tell you perfectly where each functions starts and ends?

nolaspring 6y ago

Also Requires visual studio

layoutIfNeeded 6y ago

"You may not like it, but this is what peak performance looks like."

userbinator 6y ago

I'd rather work with this than Enterprise Java. 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.

kccqzy 6y ago
This. The convenience of everything in a single file is underrated. You can do all the modern best practice like splitting into more functions, making functions small, and I wouldn't mind if they are all in the same file.
bitcharmer 6y ago
Being a Java dev (not enterprise any more) I agree that some enterprise software is an abomination for exactly the reason you mention. However, can you imagine how bad an enterprise C++ project would look like?
- rowanG077 6y ago
  A gameengine is as enterprise as it gets.

bufferoverflow 6y ago

And * 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 on 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.

isatty 6y ago
Not defending the use of magic numbers but some of it is clear for someone who has written physics code before.

MauranKilom 6y ago

This is what draws my attention more: > //FIXME: There's a threading issue in CryPhysics with ARM's weak memory ordering. https://github.com/CRYTEK/CRYENGINE/blob/6c4f4df4a7a092300d630f8f89d2ebda39183c36/Code/CryEngine/CryCommon/CryPhysics/physinterface.h#L133 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.

361994752 6y ago
This is not about race condition. Rather it is something more like why you need volatile keyword. https://stackoverflow.com/questions/72275/when-should-the-volatile-keyword-be-used-in-c https://stackoverflow.com/questions/72275/when-should-the-vo...
- MauranKilom 6y ago
  Can you elaborate? volatile and threading in C++ are orthogonal to each other. I don't understand why you consider a C# volatile discussion relevant when talking about C++ race conditions.
- ensiferum 6y ago
  Volatile in c++ has nothing to do with threading.

wesmac 6y ago

I knew which function this was before even clicking the link. I spent months working on and debugging that exact function and the surrounding CryPhysics code improving network interpolation back in 2014. All the undefined behavior caused us some trouble while porting to the "next-gen" consoles of the time. 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.