A request for a fix in collision shapes constructors

Hi everyone!

I started to use OgreBullet not long ago. The library looks very nice, but recently a came across some very confusing "features" when tried to understand why I get "access violation" exception in my demo code.

The OgreBulletCollisions::CapsuleCollisionShape and OgreBulletCollisions::ConeCollisionShape class constructors have third parameter called "axe". Why "axe"? As I figured out during the process of fixing my weird exception, this parameter stands for capsule/cone rotation axis. I don't know any case when "axe" means "axis" . Maybe it would be better to rename this parameter to "axis" or "rotationAxis" to avoid such kind of confusion?

Moreover, the above constructors contain code like this:

if (axe == Vector3::UNIT_X)
mShape = new btCapsuleShapeX(radius, height);
else if (axe == Vector3::UNIT_Y)
mShape = new btCapsuleShape (radius, height);
else if (axe == Vector3::UNIT_Z)
mShape = new btCapsuleShapeZ(radius, height);

In other cases the mShape remains equal to nullptr, causing "funny" exception when you try to pass some arbitrary vector, so as I did for the first time . I think it would be nice to use some special enum as the third parameter instead of Ogre::Vector3, if you consider only these three types of vectors. Or use default value for mShape, if none of these three vectors is the case. Or at least choose the maximum component of input vector and use it to determine the shape axis. But as for me, the first option is preferable, because it is more explicit then the other two.


Thanks.

UPDATE: Probably this request should have been posted in Ogre addons tracker, but I think the topic is a subject for discussion.

Good points all around.

1. The "axe" parameter can be confusing. I agree it should be renamed to "axis" for better clarity.

2. mShape = NULL and results in a crash. I agree this is naughty behavior. It's probably more appropriate to throw an exception that provides some meaningful information about what the ctor expects. The problem with changing it to an enum is that it's a 'breaking change'. I generally try to avoid breaking changes unless they are absolutely necessary. In this case, adding another "else if" to throw an exception is probably more appropriate. Defaulting to an axis in the case of a vector that is not aligned to one of the three positive cardinal orientations could result in even more confusion for new users as they try to figure out why the cone isn't aligning as specified. It's better to throw an exception as mentioned.

- Fish

I agree that throwing an exception is a good solution, but again, in this case it is not enough obvious for the end user that he passed an unexpectable parameter, until exception is thrown in runtime. Another possible solution is to add an overloaded constructor version that accepts third parameter as enum, and declare the current constructor as deprecated. This won't break existing applications that use library, and as far as most IDEs are able to show overloaded constructor variants in code editor, new users will notice at once, that the constructor as a subject for change.