Coding Conventions

Do not leave "trailing whitespace" at the end of lines.

Reason: In the long run, it causes unnecessary diffs when someone removes this whitespace, which many text editors (and some humans :) ) do automatically.

Do not use tabs. Use only spaces for all indentation.

Reason: Tabs are nice when they are used 100% consistently for indentation, but in the long run it’s hard to enforce using them 100% consistently esp. when many people are working on the same codebase. And when tabs are used inconsistently (sometimes mixed with spaces for indentation), problems arise — because people have different tab width in their editors, so for some people indentation will look broken. It’s much easier to enforce the rule always use spaces.

PascalCase means that the first character of each word is capitalized. Use it for all identifiers.

Reason: We just historically wrote them always as lowercase and now it would be too big effort (for too little gain) to change. Some Pascal codebases write them also lowercase (e.g. LCL, although it is not 100% consistent) so we’re not alone in this decision.

Do not mimic C "K & R" style (https://en.wikipedia.org/wiki/Indent_style#K.26R) in Pascal:

Instead, the "begin" should usually be indented the same as "end".

To look simpler, it’s OK to omit begin/end when they would surround only 1 statement:

The "else" keyword is written on a new line, unless it’s right after "end". So:

Never use with keyword. Using with makes the code very difficult to read, as some of the symbols inside the with A do begin …​ end clause are bound to A, and some are not, and it’s completely invisible to the human reader which symbols are which. And it’s impossible to determine it, without intimately knowing the complete API of class/record A.

E.g. what does this code do?

Does it modify A contents, or does it modify outside variables, merely reading the A contents? You really don’t know, until I show you the documentation (or declaration in Pascal) of the class of A, and all it’s ancestors.

Compare this with these obvious code snippets:

or

The with also makes the code fragile to any changes of A API.

The uses clause of our units and examples should follow the order

Each part should start from a newline.

Use strict private whenever you can, that is: use it for private stuff that is not accessed by other classes/routines in the same unit.

Use just private for stuff that is accessed by other classes/routines in the same unit. Basically, use private where you would use friends in C++.

This improves code readability in case of large units, that feature more than just a single class. And we have many such large units.

Using strict protected is not advised in CGE.

Reason: The distinction between strict protected and protected is not very useful for readability (regardless if something is strict protected or just protected, you must think "something outside of this class, even outside of this unit, may access it"). Moreover, it is forced downward, on all descendants of this class (that must then differentiate between overriding in strict protected vs protected, which is uncomfortable because the decision whether to use strict protected or protected should be an internal (implementation) decision within the ancestor, not affecting the descendants).

So, use just one protected section, do not bother splitting it into strict protected and protected.

Use Json, Url, Http etc. in Pascal identifiers. Not JSON, URL, HTTP.

Reason: It looks more readable in long identifiers, e.g. GetHttpResponse is more readable than GetHTTPResponse.

Note that this rule applies only to the Pascal identifiers. The Pascal identifiers are CamelCase, and so they warrant writing abbreviations inside like words, Json, Url, Http. Normal English text (e.g. in comments and error messages) should follow English conventions of writing the whole abbreviation uppercase.

If you use the nested types / constants, indent the fields inside the var block as well. See the example below, notice that MyField is now indented more than in the example above. Admittedly it’s not a perfect convention — MyField indentation is now inconsistent with MyPublicField. But on the other hand, MyField indentation is consistent with MyNestedConst and TMyNestedClass and how you usually indent var block.

All the engine functions are "reentrant", which means that they are safe to be called recursively, even through your own callbacks. E.g. the TFileProc callback passed to FindFiles can call FindFiles again inside it’s own implementation, causing a recursion.

We use standard FPC and Delphi compilation symbols like:

to differentiate between compiler versions, and some more.

We detect Delphi by just lack of FPC, e.g. property Foo: Integer …​; {$ifdef FPC}deprecated 'Delphi does not support deprecated clause for properties';{$endif}.

See castleconf.inc (included in every CGE unit).

We also use DEBUG symbol. The build tool, when compiling in debug mode (--mode=debug), defines the DEBUG symbol, and adds some runtime checks, like range checking and overflow checking. You can use {$ifdef DEBUG} in your own code to add additional things. There’s also the RELEASE symbol, but usually we don’t check for it’s existence — if DEBUG then we’re in debug mode, else we’re in release mode.

This is good:

Guidelines:

ObjectPascal is a hybrid OOP language and it has global function pointers and method pointers. They are incompatible, since the method pointer is actually two pointers (the class instance, and the code address). When designing a function that takes a callback, you’re faced with a problem: define "a pointer to a method" or "a pointer to a global function/procedure"?

In the past, I often chose to use "a pointer to a global function/procedure". With a generic "Data: Pointer" parameter, to allow passing user data. This is easier to use when you don’t have a class instance (and you don’t want to create a dummy class just for this), and it’s always allows to add overridden version with "of object" callback (passing object instance as the Data);

Nowadays, I usually define "of object" callbacks, assuming that all non-trivial code is usually in some class, and the "of object" is more natural to be used in OOP.

Place the implementation of constructors (Create*) first, then destructor (Destroy), and then the rest of methods. I do not have a precise rule about the ordering of the rest of methods — I usually like to group related methods together.

Standard StrToFloat in FPC and Delphi converts floats to/from Strings using locale-dependent DecimalSeparator value. On some systems (e.g. on Polish Windows) it is equal to comma (,), not a dot (.). This is usually not what you want: when you read/write files, or command-line arguments, you usually want to have "dot" as the only decimal separator, so that your application works regardless of user’s system locale.

So instead use StrToFloatDot. As a bonus, it is also marginally faster.

Same advise applies for related functions:

Embarcadero decided to make things weird: https://docwiki.embarcadero.com/RADStudio/Sydney/en/Simple_Types_(Delphi). The LongInt / LongWord are

This is

So just don’t use these types in CGE code.

Traditionally, Extended used to be a 10-byte floating-point type, available in old Pascal compilers like Turbo Pascal. But modern systems don’t support such 10-byte floating-point type natively, so both FPC and Delphi actually make it an alias to some other type (usually Double, standard 8-byte floating-point type) on most (but not all) platforms.

The exact rules that define "what exactly is Extended" depend on the compiler. Basically, both FPC and Delphi try to alias it to the best (most precise) floating-point type that can still be efficiently implemented on given OS/CPU combination.

The rules are:

The end result makes Extended not very useful for general cross-platform (and cross-compiler) code, due to this uncertainty. You cannot rely that Extended has 10-byte precision, and if you use Extended a subtle bugs may creep in due to not testing on all possible Extended situations. And GPUs don’t support anything above Double anyway.

So we recommend you just use Single for most of code, and eventually fallback on Double if you must have great precision (which usually means you proved that some calculation goes badly with Single and switching it to Double is a reasonable fix).

For CGE developers: While the above recommendation holds (do not use Extended), if you really have to use it, then the castleconf.inc defines:

On FPC, we follow the same approach to String as in Lazarus: String is an alias to AnsiString, and it should always contain UTF-8 data. We use necessary compiler switches to make String = AnsiString, and the CastleUtils has necessary initialization to make sure that strings can just carry UTF-8 on all platforms.

See FPC docs:

On Delphi, we follow the standard approach of modern Delphi: String is an alias to UnicodeString, and it contains UTF-16 encoded data.

See Delphi docs:

Correspondingly, Char is 8-bit with FPC, and 16-bit with Delphi. And PChar points to 8-bit characters on FPC, 16-bit on Delphi.

With both compilers, you can explicitly use AnsiString to request 8-bit string. And AnsiChar for 8-bit character, PAnsiChar to have a pointer to them.

What to do?

Our unit CastleUnicode contains various helpful things to deal with strings in a way that it Unicode-safe and works with both FPC and Delphi painlessly. In particular, use:

While most of the finalization code should usually go to the destructor (Destroy), it is OK (and sometimes even desirable) to override BeforeDestruction and put there code that finalizes things, but keeps the object state consistent. The idea is that finalization can be split into 2 phases:

100% of class implementation must be ready to work during BeforeDestruction. So

As a bonus, implementing BeforeDestruction is sometimes easier than implementing Destroy: Because BeforeDestruction is only called if the class constructor reached the end, i.e. fully initialized everything (see Delphi docs of TObject.BeforeDestruction). This is in contrast to implementation of Destroy, that has to be very careful, because destructor can be called even on half-initialized object state (because destructor is called even when the constructor fails with an exception).

Once the actual Destroy starts, the object may be in half initialized state and in general it is not guaranteed that operating on such object works. The object may have some critical fields cleared (uninitialized), and in general methods/properties may assume they are initialized. E.g.

We often make implementation of some class "robust" to work even when this class is in the middle of Destroy. But there is no guarantee about it and we do it on a case-by-case basis (when it is needed by something). In general, class implementation may assume that it is never called in "half initialized" state, and that user code checks if not (csDestroying in SomeObject.ComponentState) then…​ if it could happen when SomeObject is in the middle of Destroy operation.

In the interface, put a blank line before a comment in

sections. Unless that comment is preceded by the line with section keyword (like public).

Putting a blank line before a comment in a private or strict private section is allowed but not required. Often you want to make these private sections more terse (also there is no need to strictly document everything private) so it’s OK to not use blank lines.

Do not add blank lines before the line with section keyword (like public). Do not add blank lines before every undocumented (private) method / property. E.g. this is good:

This is not good:

A typical setter implementation should check whether the new value is different than the previous, and early abort (not do anything) when they are equal.

Reason: This is an easy and useful optimization. The cost of checking for equality is usually trivial and it can save non-trivial operation cost. Do this even if you suspect (or know) that layer below is also doing such checks. Don’t trust the authors of "layer below" (esp. if it’s an external library).

Implementing setters like this from the start is much easier than latter adding such conditions. If your setter doesn’t make the check if Xxx <> Value then, then later developers will be afraid of adding it, fearing that it breaks something (by not re-initializing something in some cases). So we generally require to implement setters like this from the start.

Our convention is to do it by if FXxx <> Value then followed by the FXxx := Value; assignment (with the FXxx always on the left side, so it’s easy to see that comparison and assignment work on the same identifiers). Like this:

Lazarus CodeTools generate a setter with early exit using Exit instead, we discourage this setter form (not because it’s inherently worse, but just the above is also good and it is prevalent in CGE sources, so better follow existing convention):

All protected, public, published identifiers should be documented (unless you really don’t have anything to add that isn’t already spelled out by the identifier name).

Documenting the private and strict private identifiers is also encouraged if there’s anything that isn’t obvious. We don’t strictly require documenting them — as their meaning / purpose is often obvious enough from the name, and the details can be inferred by analyzing the implementation code. But if you’re unsure, it is better to write docs.

Do not make a documentation that just repeats what the identifier already says. If you define property RotationAxis: TVector3 …​ then documenting it like this isn’t very helpful:

If this is really everything you have to say, then you can skip the documentation in such case — there’s no point in writing exactly the same words that the identifier is already composed of.

But there’s almost always something more that you can add that is useful.

This is a better example:

As for English:

Lazarus IDE, when editing, adds automatically a bit useless comment with just a class name, { TClassName }, before each class. They are not useful (a person reading API documentation generated by PasDoc, or reading source code, will see the class name anyway nearby). And these comments obscure the good documentation string for PasDoc. (See https://github.com/pasdoc/pasdoc/issues/149 for discussion about it too.)

I recommend to disable this ("Header comment for class") in Lazarus settings, see the screenshot in https://github.com/pasdoc/pasdoc/issues/149#issuecomment-1308837496 .

In Castle Game Engine we use the same names for OSes (operating systems) and CPUs (processors) as FPC. FPC in turn follows names used by GNU tools.

Reasons, in short:

We acknowledge that some of FPC (and GNU) names for OS / CPU are not "optimal". Users sometimes prefer (are accustomed to) a different name. Sometimes FPC naming is justified by current technical reasons, sometimes it is only historical. So in the documentation / UI / website, you are welcome to write longer names (e.g. instead of "Darwin" write "macOS (Darwin)" ).

Examples:

FPC 3.3.1 performs "case analysis": whether all possible input values are accounted for in the case clauses. It means that:

There are advantages of this analysis, though it’s also another complication when writing code — esp. because we have to account also for compilers that don’t do this analysis (like older FPC and Delphi). Our castleconf.inc defines COMPILER_CASE_ANALYSIS for new FPC.

So how to write your case?

For some large units, it is reasonable to split them into multiple include files. The structure to do it looks like this:

and then in each include we do this

This is not really our idea — at least FPC source code also uses a similar trick for some units.

The {%MainUnit castlexxx.pas} is a comment for the compiler, but it is useful for the Lazarus CodeTools (used in both Lazarus IDE and also in our VS Code extension) to properly complete code when inside an include file.

When is such organization useful?

To make our bash scripts portable, use this shebang (first line of a shell script):

This shebang is more portable than often-used #!/bin/bash — as it works reliably also on FreeBSD and OpenBSD. In total, we want our bash scripts to work on a variety of systems: Linux, FreeBSD, macOS, Windows (with Cygwin or MSYS2) etc.

So never use #!/bin/bash as shebang.

Also never use #!/bin/sh. On most Linux systems, this is not equivalent to bash, instead it points to a shell that is a bit more lightweight but also quite more limited than bash. In CGE, performance of our shell scripts is really not critical all, so we should just use bash and have more maintainable scripts.

Read and follow the Use Bash Strict Mode (except the shebang shown there as example).

The set and IFS configuration described there make writing and maintaining shell scripts really much easier. Essentially bash behaves a bit more like a programming language.

Summing it up with the previous advise, this is what your scripts should start with:

For historical reasons, some of CGE scripts may use something a little more relaxed:

Usually this is just a historical thing and will be upgraded in time. We should follow Use Bash Strict Mode fully going forward (except shebang, which we want more portable).

For many non-critical tasks, we use bash scripts for Windows too. Both Cygwin and MSYS2 provide bash and many other Unix tools for Windows.

For some critical tasks though, really used by many people (like building CGE build tool), we don’t want to assume that user has Cygwin / MSYS2. In this case, we use PowerShell on Windows.

Note: GitLab CI also uses PowerShell on Windows machines.

Pascal is a language with manual memory management, i.e. you have to remember to free the things you allocate.

It’s often quite easy to manage this, assuming that other code "plays nice":

In short, things are easy when nothing else frees the object you were supposed to free. Without explicit allowance, the other code should not free the things you created.

This guideline is thus: When you create an instance of something, then you decide when and how it should be freed. Nothing else should free it, unless explicitly permitted by the documentation.

This includes the cases when you receive an instance of something by accessing some property, function or class function. Do not free it, unless it is explicitly documented that you should free it. Only free things where you have called constructor.

This rule has some additional consequences:

Technically, in Pascal a value of an object instance (like X: TMyObject) can always be nil.

But this guideline limits the cases when you have to worry that something is nil, in order to avoid writing lots of checks X <> nil in the code.

Follow this:

In general prefer declarative API (properties) over imperative (methods, esp. with complicated usage scenarios).

The classes that expose the "solution" as a set of properties are simple to use. Exposing a "solution" as a set of methods (that must be called in some specific order for the desired effect) is usually not as simple. Of course this is just a general guideline, I’m sure you know lots of exceptions to this rule! CGE itself is a big OOP library with lots of classes with lots of properties and lots of methods. If something is naturally an "action" ("draw it now!") then it should be a method (Render). But if something is a "state" ("use this color when drawing") then it is a property (property Color: TCastleColor; property Text: String; works better than procedure Draw(const Color: TCastleColor; const AText: String)).

Properties should work independently. Property value should not be "automatically" set by setting another unrelated property (e.g. setting TCastleUserInterface.HeightFraction does not set also TCastleUserInterface.Height) or by doing something (e.g. adding a control does not set its TCastleUserInterface.Height; it represents "the desired height", and programmer should instead read EffectiveHeight to know the resulting height). Of course there are exceptions to the latter — some methods naturally set some properties, but then it should be clear that given method does this, e.g. TCastleViewport.Setup2D sets Viewport.Camera.ProjectionType.

You want getting/setting properties to work naturally, regardless of the order in which it happens (this is nice for the programmer using the API, and necessary for reliable deserialization).

Properties should generally work like you would expect a variable works. E.g. reading a property right after setting it should result in the same value. Setting property multiple times to the same value should have no effect. See the Pascal guidelines on Properties (modern Pascal introduction) : …​it’s a good convention to design properties to behave more-or-less like fields:…​.

Classes with independent properties are simple to use — both from CGE editor (that exposes any published properties of TComponent descendants), and from code (code that sets a few properties is obvious to follow).

If you want to suggest some optimization (of speed, of memory usage) to the engine, especially if it:

→ then always first do some tests/thinking whether it’s really worth it.

There are many situations where optimizing is not a good idea, because it will not change the "bottleneck" code (which means that the speed / memory use of something else is so large (in comparison) that it completely "masks" the thing that you optimize, making it irrelevant). In such cases, optimization is actually harmful, because the code quality goes down — the optimized code is usually longer and/or more convoluted.

Bottom line:

As you can see, I put more emphasis on thinking about code quality than optimization. That is because I see some of us often making the mistake of not caring about code quality enough, and instead rushing to make an optimization (that lowers code quality for little-to-no gain to the final applications).

Of course, this does not mean we don’t want to optimize. It just means that we require justification for each optimization, the optimization must have a noticeable effect on some real-world use-case. We want the code to be fast and use little memory — there are various ways to achieve this, often using smart algorithms on CPU, and/or thinking about how the CPU cache is used, and/or delivering data in better chunks to GPU. Low-level optimization of some local routine is often not the most effective approach.

There is also a dreaded "death by 1000 cuts" that we want to avoid, which is sometimes caused by missing a number of small optimizations that would have a noticeable effect overall. E.g. that’s why we use "Single" throughout the engine code, not Double or Extended. (except some special code where we have testcases that "Single" precision is not enough). Using "Double" everywhere would have a noticeable negative effect on the speed (yes, I tested it long time ago). But e.g. obsessively avoiding calling Sqrt in the engine…​ proved to be usually a useless optimization, causing various bugs and not achieving any speed gain.

So, there are cases to be made for some low-level optimizations. But don’t fall into the trap of implementing lots of useless low-level optimizations blindly.

We want the compilation of the engine, and all examples, to pass without any warnings. On all supported platforms and with all supported compilers.

We consider compiler warnings as generally useful, so we don’t want to mindlessly hide all warnings (or get into a habit of ignoring them). We want to obey the warnings. A warning is a TODO — you need to deal with this, maybe by improving the code to be more reliable.

Sometimes the warning isn’t really valid, but you still should deal with it. It is acceptable, in justified cases, to selectively turn off the warning, like this:

We still consider this a "win": while you will occasionally have to deal with useless warnings from the compiler, but on the other hand you will pay attention to compiler warnings and the compiler will help you detect real problems in code. The warnings reported by the compiler are often about "edge-cases", situations that don’t happen often (or maybe even you think they will never happen), but sometimes the compiler is right: that special situation can happen, and the code should account for it.

Ideally, we add new features to the engine by just "naturally" extending the existing API. By "naturally" I mean that the new concept (class, method, property, whatever)

The engine API should be consistent (with itself).

Example: If you ever need 4x3 matrix, make it consistent with existing TMatrix4. Do not declare it like this:

Can you spot all the inconsistent things in this TNewMatrix4x3 declaration above? That would make using TNewMatrix4x3 weird alongside the existing TMatrix4? This would be much better:

Sometimes adding new things without breaking old things, in such way that the end API is consistent, is not possible. We didn’t predict everything when designing CGE API :) In this case, we have to change our API, to incorporate the new feature, such that the resulting engine API feels consistent. We don’t want features to reinvent some concepts, e.g. because some detail in the existing concept was not perfectly suitable. We want to adjust existing concepts to be better.

In this case, when the API of existing concept must change, try hard to keep the old API still available. Use deprecated Pascal directive, use it with a string parameter that explains shortly the advised alternative, like

It works nicely, will display a visible warning to users that they use the deprecated feature, but at the same time Foo will continue to function normally.

But there are rare cases when providing backward compatibility is not possible. Or maybe it would be possible, but would be a lot of work on engine development side (to make it / maintain it), or the existence of "compatibility crutch" would be too confusing to new users. In this case, after considering all the options, I say it is acceptable to break backward compatibility. You’re doing it for the greater good. The engine must have room to evolve, to be better and better, we must have a way to change old/bad/un-optimal API design into new API design. This is the only way to keep our API consistent and keep adding new features. IOW:

Think about how the engine will look in a few years. Think about the perception of new people that learn the engine. If the engine has now 100 users, think that it will have 10x (1000) users in a year. We don’t want the new 900 people to suffer from bad design decisions we make today, and suffer because we could not fix these decisions out of fear of breaking backward compatibility.