Request: various code design improvements

[mreuvers]

Hi there,

We've worked over half a year with Bullet now and it's safe to say that there are a number of constructions and formatting issues in Bullet that, well... are not so transparent

I'd like to start this thread on possible code improvements, simply to share my ideas of improvements with others as well.

A non-complete list, in random order. Feel free to add suggestions here as well!

LinearMath
Apart from all the math functions, the current setup also contains several helper files that really don't have anything to do with linear math. E.g., btAlignedObjectArray.h, btAlignedAllocator.h, btIDebugDraw.h, btRandom.h and a bunch more. Perhaps it's an idea to introduce a helper folder? Or even more finegrained: a helper and a memory folder? Math = math, file access = file access, memory = memory... anyhow, you get the point

btScalar.h contains setup info and helper functions
Simply look at btScalar.h and you'll see what's wrong with it. My guess is that this is the result of evolutionary programming, but it makes absolutely no sense to define asserts, version info, platform specific compiler pragmas, operator new overloads etc in that particular file. Perhaps it's an idea to clean it up and separate it over various other files (and NOT place them in LinearMath )

assert inconsistent
The library contains several calls to btAssert, but also various calls to assert.h. It would be best to use one consistent name: btAssert. Furthermore the current way of nullifying the asserts e.g.:

Code: Select all

#define btFullbtAssert(x)

is not entirely correct. Please check this little gem http://powerof2games.com/node/10 for more info on how to properly implement asserts.

Namespaces
Unfortunately bullet doesn't seem to use any namespaces. Is there any reason for this design? IMO it would be more logical to at least setup the namespaces in the same way you organize the directory structure.

No smart pointers
Currently bullet uses raw pointers all over the place. From a memory safety perspective, perhaps it would be an idea to look at the Boost smart pointer implementations?

Inconsistent naming conventions
Bullet seems to be adopting the Camel Casing convention, however there are several occasions in the code where this is not the case. For instance look at btDbvt.cpp, this class contains functions that are called recursedeletenode, removeleaf etc, making it hard for a programmer to read.

Inconsistent indenting
The code contains several examples of non-consistent indenting. Simply look at btDbvt.cpp again. The implementation starts at column 1. That file really is a mess to be honest Perhaps it is an idea to run an automated formatter like:

http://astyle.sourceforge.net/

on the complete trunk? Never tried it though, but perhaps it prettifies the code in such a way that it is more readable for everyone.

C-Style casts all over the place
Apart from a few static_casts, the code hardly contains any C++ like casts. A good example of potential pitfalls for that:

http://www.informit.com/guides/content. ... seqNum=134


Inconsistent usage of constness
There are a lot of methods out there in Bullet that could be declared const, look at all the getters for instance. However this seems not to be enforced. Some getters simply aren't declared const. The other way around: in some cases I even found setters that were declared const, making me wonder why they were called setters in the first place.

Max chars per line
In our company we maintain a max character limit of 100 characters, forcing coders to write code that is readable without scrolling. It's an easy rule that can be easily enforced by configuring your editor. In Bullet there are lines that go well beyond the 200 characters, making it hard for programmers to understand the code.

That's it for now. I'll save the rest for later Please do note that this is not an attack in any way on Erwin or anyone who contributed to Bullet. Bullet is a great product, but if you look at it realistically you cannot deny that it also suffers from a lot of design and formatting issues. My idea is to get an overview of these weaknesses so perhaps we can draw a roadmap of some sort to address them.

Cheers,


Martijn

[Erwin Coumans]

Thanks for the feedback.

Several of the suggestions should be applied to Bullet 2.x, so we track this cleanup issue. We gather bigger changes for Bullet 3.x, where we don't worry about API compatibility. To be clear: this discussion only applies to the core Bullet source code in Bullet/src, so it doesn't apply to other folders, such as Extras.

Inconsistent indenting
Inconsistent naming conventions
assert inconsistent
#define btFullbtAssert(x)

Accepted, all code in Bullet/src should adhere to our coding style guide, with naming convention, proper indentation. Also it should only use btAssert and not 'assert'. Thanks for the link on empty asserts in release mode, we review it.

LinearMath btScalar.h contains setup info and helper functions
Max chars per line

Accepted, and this is planned for Bullet 3.x. Indeed, several parts of LinearMath should be moved to a separate folder. There is no plan to grow this functionality, so a small folder called Common, Base or Platform should do the job. We will discuss 'max chars per line' during discussions for Bullet 3.x.

NamespacesC-Style casts all over the place
No smart pointers

We chose to use the bt prefix to avoid naming collisions, and are happy with this solution. There are no plans to introduce namespaces in Bullet 3.x. C-style cast haven't caused real issues so far, but if it becomes an issue we can reconsider.
On the smart pointers: we don't want or plan to have any dependency on STL or boost, and there is no reference counting in Bullet 2.x or 3.x. If memory safety ever becomes an issue, or we want to introduce reference counting we might re-consider this. Note that there is a shared_ptr implementation in Extras\MayaPlugin\shared_ptr.h.

Inconsistent usage of constness

Can you provide specific, including filename/line numbers?

Thanks for the feedback,
Erwin

[Prompt]

What about coding style?
I read some books like Code Complete and in my job it's really important to have coding style and well documented the source code for APIs. So I want to extend this thread with the creation of a "template" source code that includes doxygen / Qt doc style (the best ever I seen API doc) and comprehensive code style.

For example I see today in CProfileManager (I know that file is not good example) this:

Code: Select all

int i;
for(i=0;i<Some_Thing_I_Dont_Remember;i++) {
    //Do many things with no spaces and really hard to read
}

It's more comprehensive, optimized and more clear to read:

Code: Select all

// Note: use always ++i is better than post increment i++, that create a temp copy of i
//! Print info of all profile nodes... blah blah.
for ( int i = 0; i < someThings; ++i )
{
    // Do many things
}

[Erwin Coumans]

Except for removing warnings and errors, we are not putting much effort in improving the Bullet 2.x coding style, because we want to focus efforts on Bullet 3.x.

Thanks,
Erwin