New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Development guidelines #839

Open
only-a-ptr opened this Issue Apr 23, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@only-a-ptr
Member

only-a-ptr commented Apr 23, 2016

Our coding guidelines need a new home (wiki is retired) and extension. I suggest incorporating them to our Doxygen docs and adding usage guildelines for C++ features and other techniques/patterns. Example: http://llvm.org/docs/CodingStandards.html

Draft

  • 'using' directives are forbidden in headers and generally discouraged. Use explict namespace qualifiers.
  • 'auto' keyword should be avoided except for working with STL containers. Use explict declarations.
  • C++ standard RTTI is forbidden. Use a custom one like LLVM style RTTI
  • Singleton pattern is forbidden. We're not making a library. Use plain objects in static memory.
  • Memory management rule#1: If you know the object/data size, use static or stack memory. Don't allocate dynamically without a good reason.
  • Memory management rule#2: If rule#1 cannot apply, use a std::unique_ptr to manage the memory. Only use std::shared_ptr if there is actual shared ownership!
  • Memory management rule#3: If rule#2 doesn't apply, employ the RAII pattern.
  • Do not 'throw'. Use exception handling only to deal with their use in libraries.
  • Do not use boost. It looks lame in game projects. Use std (it's largely boost-ed now, anyway...).
  • When calling class methods, always use explicit 'this', to distinguish methods from free-functions.
  • Only do bare minimum of work in constructors and destructors. For complex inits, use factory functions.
  • Exception specifications (i.e. 'throws()') are forbidden. Read http://www.gotw.ca/publications/mill22.htm
  • Use 4 spaces for both indentation and alignment. Tabs are forbidden.
  • Always use {} with if() and loops, to avoid bugs like this.
  • Name bool variables using short yes/no question, for example m_is_initialized instead of m_initialized.
  • Properties are obfuscated getter/setter functions, don't mimick them in C++.

Terminology

  • nodes and beams Building blocks for our soft-body physics structures. Nodes are infinitely small ball joints with mass, beams are weightless springy connections between nodes.
  • softbody A soft body. Just the physics structure, without any usage-specific attributes.

Opinion shaping resources

Useful links

Random ideas:

  • Mikadou's memory usage guidelines are great: https://gist.github.com/mikadou/1578da33c2c36fdb308af54200764a00
  • We're making a monolithic end-user application, not a library. Therefore we don't need perfect library-like external (or even internal) interfaces. This includes usage of singletons (totally avoidable) and interface classes (mostly avoidable).
  • We're making a game. A game is different from general-use software. Games should be lightweight. This isn't a call for performance optimization. We should maintain stability and readability, but only by means necessary. See https://en.wikipedia.org/wiki/YAGNI.
  • RoR has 2 areas: 1. startup + main menu (singlethreaded), 2. simulation (multithreaded). These areas require different thinking. Startup needs to set up many various subsystems, while the simulation loop is a single monolithic system. And the code should be modelled accordingly. Data shouldn't be scattered among *Manager, like RoR currently does (map stuff...).
  • memory allocation: C++'s pattern is to make classes and dynamically allocate instances just to nicely call the constructor in place. Practical outcome: a lot of allocations for tiny memory amounts, often for objects which are only created once (singletons or not). Let's prefer making class instances plainly on stack. This involves need for minimal constructors + external init() method.
  • boost library - should be avoided. On technical side, it's avoidable, on human side, it's generally considered lame to appear in a game. Opinions by author of OGRE 2.1 link1 link2
  • exceptions and RTTI - generally frowned upon in anything striving to be lightweight: http://llvm.org/docs/CodingStandards.html#do-not-use-rtti-or-exceptions TODO: test this on actual RoR! Comment out the few uses of exceptions we have, build, compare binary sizes and test performance.

Code cleanups to do

  • Settings.h:
    • Scrap the singleton, use free functions which access module-local variables.
    • scrap the BSETTNG() macros, use global functions like GetSettingBool()
    • replace map<string, string> with map<string, Entry> and define meaningful struct Entry {Type type...}
  • Settings.cpp:
  • Please remove this cruft ( usage )
  • ScopeLog: [ header source ] = Dead and mis-designed anyway.

Already done :)

@only-a-ptr

This comment has been minimized.

Member

only-a-ptr commented Apr 23, 2016

Things that shape my thinking:

@mikadou

...makes sense as it is now available in the official standard. For other stuff not (yet) in the standard...

IMO "it's in the standard" cannot be considered as quality or usage guideline anymore. C++ is a language designed by a commitee, which generally is a reliable way to create a catdog. The C++ specific outcome is that with every revision, the number of inconsistencies and gotchas in the language grows exponentially. I recommend watching https://youtu.be/smqT9Io_bKo?t=19m6s and https://youtu.be/wQxj20X-tIU?t=1m14s (2x Scott Meyers)

@mikadou

This comment has been minimized.

Contributor

mikadou commented Apr 24, 2016

👍 for moving coding guidelines to developers documentation. Overall I believe guidelines should be very concise containing only the most relevant items. If the information is to verbose noone will bother reading it anyway.

Some comments on your ideas regarding techniques and patterns, though:

  • Granted, RoR is not a library. But why does this imply it has to be build as a monolithic bulk without modularity? In fact the core mass-spring simulation might be a good candidate to move into a dedicated library, don't you think?
  • Again, RoR is a game, and games certainly do have special performance requirements. But other applications (e.g. trading applications) do, too. If you're already using C++, it is pretty likely that you care about about performance. It should be carefully evaluated if and where it makes sense to deviate from generally accepted methodology like RAII. I read YAGNI as do not plan ahead too far, because things are going to change anyway. Development is a dynamic process and prioties may shift. I don't think one should use YAGNI to justify not implementing proper interfaces and encapsulation (continuous refactoring is the key).

C++'s pattern is to make classes and dynamically allocate instances just to nicely call the constructor in place. Practical outcome: a lot of allocations for tiny memory amounts, often for objects which are only created once (singletons or not). Let's prefer making class instances plainly on stack. This involves need for minimal constructors + external init() method.

  • I might be misunderstanding this, but as far as I know it is generally considered good practice to create objects on the stack (except for very large objects, and those which need to be transferred to different owners at runtime). Saying it is C++'s pattern to dynamically allocate instances sounds weird to me.
  • How does this require minimal constructors + external init() methods? I could imagine this pattern to be used if one actually disables exceptions, but otherwise why not do the work in the constructor itself? Speaking of exceptions and RTTI and their overhead: How does disabling these features impact linking against dependencies?
@only-a-ptr

This comment has been minimized.

Member

only-a-ptr commented Apr 24, 2016

If the information is to verbose noone will bother reading it anyway.

Oh he will, because I'll link him there the moment he breaks a rule.
The point is to settle on something and avoid unclarities. It's primarily for ourselves, not for new guys.

Granted, RoR is not a library. But why does this imply it has to be build as a monolithic bulk without modularity?

We don't need modularity. Libs do, so stuff like Managers works for them. In RoR, everything is related anyway, and I want the code to be truthful about it.

I'm not worried about 'Monolithic bulk'. I can refactor that to pieces if necessary. Everyone can. But 'Modularity' sounds suspicious. I've seen a lot of pseudo-modularity which wasn't really modular but
pretended to be, just because authors thought it's more OOP that way. IMO this is worse that honest bulkiness.

RoR itself is a bad case of OOP over-engineering, misunderstanding and abuse. One thing I scrapped was interface 'IManager' with methods like getMemoryUsage() and implementations like {return 0; /TODO/}. Bah!

the core mass-spring simulation might be a good candidate to move into a dedicated library, don't you think?

Actually, I think the very opposite: we should explicitly state that we never intend to separate our physics core as a library.

First off, time budget. We hardly have enough time to push RoR, and complicating our work by defining and respecting a general-purpose interface around our physics core is avoidable expense. RoR is ACTUALLY popular because it has nice trucks and nice cars, not because of the physics. We need linkage between physics and GFX.

Second, quality. If someone wanted a softbody sim code in C++, he's MUCH better off writing his own. Our physics have a potential, but they actually suck, vehicles are super springy and too light. We lack tools to deliver good physics models. Our truckfile is *** mess.

Finally, I'm a big fan of http://geometrian.com/programming/tutorials/write-games-not-engines/. And it's not just attitude, I've actually seen many many unfinished and half baked engines/libs where somebody wasted a lot of time.

It should be carefully evaluated if and where it makes sense to deviate from generally accepted methodology like RAII. I read YAGNI as do not plan ahead too far, because things are going to change anyway. Development is a dynamic process and prioties may shift. I don't think one should use YAGNI to justify not implementing proper interfaces and encapsulation (continuous refactoring is the key).

No offense, but you sound like a textbook. These rules do sound very rational and convincing, but in reality, they don't work, and worse, they warp into imitations of themselves just because programmers won't accept the fact they don't work. What's "proper interfaces and encapsulation"? In RoR, for instance, some smart guy implemented truckfile loading and parsing in Beam() constructor, probably because "if the parsing fails, there won't be a Beam". I know this is an obvious case, but it starts here. Similarly, before I intervened, the game startup involved huge RoRFrameListener() constructor and several GameState* classes, used poorly and for no purpose. I replaced that with a plain MainThread::go() method. Just scroll back in GH history.

As the "make games, not engines" article above reveals, programmers have a tendency to hugely overthink generalization and modularity. It's something to expect and stand against. Poor OOP and fake modularity already caused enough problems in ROR.

Saying it is C++'s pattern to dynamically allocate instances sounds weird to me.

Sure, objects which only live in a function call, like Ogre::Vector3, are always on the stack. I'm talking Manager/Factory objects. Maybe it's just me, but whenever I see those in code, they're always dyn-allocated, most often as singletons, even though they mostly weight just a few bytes.

How does this require minimal constructors + external init() methods?

Check this: https://github.com/RigsOfRods/rigs-of-rods/blob/master/source/main/Application.h. I wrote this class to get rid of singleton infestation of older RoR codebase. I'd like not to have such "globals", but the codebase is too warped around them. Those classes need to be initialized at a specific moment, which is done in the ctor. In order to have them on stack, one would have to either assign a new instance to them (provided the = operator is default or acts the same way) or add separate init() method which gets called at the right moment.

@only-a-ptr

This comment has been minimized.

Member

only-a-ptr commented Apr 27, 2016

OK, something specific to discuss:
https://github.com/RigsOfRods/rigs-of-rods/blob/d1d3d0f3c626c80f49b745ef0636202334acdbff/source/main/gameplay/PlayerColours.h
This is a singleton - a dynamically allocated object - that has no data in it! All of it's methods could have been static methods or C-style free-functions. But apparently it's author loved singletons, so he dyn-allocates it just to fullfil the pattern:

// initiate player colours
PlayerColours::getSingleton();

What do you suggest we do with crap like this in our codebase?

I'm for C-style free functions:

// public function - in header
updateMaterial() -> UpdatePlayerColorMaterial()
getColourMaterial() -> GetPlayerColorMaterial()
getColour() -> GetPlayerColor()

// protected
updatePlayerColours() -> InitializePlayerColorMaterials()
    //  because it's only called once, on init, anyway.
@only-a-ptr

This comment has been minimized.

Member

only-a-ptr commented May 4, 2016

Another thing: Terminology. Specifically: How do we refer to what's commonly called "truck" (an instance of class Beam) in code?

RoR used to be truck-only, so all the identifiers were called trucks, trucknum, getCurrentTruck() and such. That doesn't feel right, we have more than just trucks. Cargo boxes aren't trucks. TDev called the encapsulating class Beam, but that's duplicate with "beams" as connections between nodes. So far, I used identifiers such as Beam* vehicle or Beam* rig, but none of it stuck.

My suggestion: Let's treat physics bodies as separate things and vehicles/machines as another thing which "uses" physics bodies. Let's call our physics bodies simply softbody and our vehicles/machines/loads/any other objects... I'm thinking actor, suggestions are welcome.

@only-a-ptr only-a-ptr changed the title from Coding guidelines to Development guidelines May 4, 2016

@AnotherFoxGuy

This comment has been minimized.

Member

AnotherFoxGuy commented Nov 19, 2016

🚧 for the git commit messages style

Inspired by atom and commit-message-emoji

"Add" =>
"Cleanup" => ✂️
"Codechange" => 📝
"Change" => ✏️
"Doc", "Documentation" => 📚
"Feature" =>
"Fix" => 🐛
"Merge" =>
"Prepare" => 🔧
"Release" => 🎉
"Remove" =>
"Revert" => 🔙
"Sync" => 🔃
"Update" => 🎌
"WIP" => 🚧


- `` when you added things - ✂️ `✂️` when you did a cleanup - 📝 `📝` when you did a change to the code only, which doesn`t effect gameplay in any way (so no bugfix!) - ✏️ `✏️` when you made a change (use it as little as possible, unclear entry) - 📚 `📚` when the changes are only regarding documentation - `` when you added a feature - 🐛 `🐛` when you fixed something - `` when you merged a pull request - 🔧 `🔧` when it`s something that prepares a bigger change - 🎉 `🎉` when a release is... released - `` when you remove a branch/file - 🔙 `🔙` when you dare to revert something - 🔃 `🔃` when you are syncing a branch - 🎌 `🎌` for language commits only - 🚧 `🚧` when something is work in progress(WIP)
@only-a-ptr

This comment has been minimized.

Member

only-a-ptr commented Nov 19, 2016

@AnotherFoxGuy Thanks for the nice idea and suggested icons. However, I'd like to change the commit style even further. We have a lot of labels, but in practice, I don't recall seeing anything but [add/feature] [codechange/code] [fix/bugfix]. So instead of using labels to describe type of change, let's use them to describe the affected area of codebase/user experience. The world is always fuzzy.

  • 🚧 :construction: Work in progress (WIP).
  • 🐛 :bug: Bugfix.
  • 🎮 :video_game: An adjustment/change of gameplay.
  • 🔧 :wrench: Internal change (codechange) which doesn't affect user experience.
  • :sparkles: New feature.
  • 📚 :books: Change of documentation only.
  • 📐 :triangular_ruler: Code cleanup (codechange only)
  • 🎌 :crossed_flags: Language only commit
  • :white_check_mark: Merge of pull-request (PR)
  • 🔙 :back: Revert commit
  • 🎉 :tada: Release!

This list is not final, I'm still thinking about it/accepting suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment