[2.1] ObjectId small improvements

Minor issues with the Ogre API that can be trivial to fix
Post Reply
User avatar
Klaim
Old One
Posts: 2565
Joined: Sun Sep 11, 2005 1:04 am
Location: Paris, France
x 56
Contact:

[2.1] ObjectId small improvements

Post by Klaim »

I didn't yet integrate Ogre 2.1 in my project (I miss some features/related projects to update before finally being able to do it)
I was reading https://bitbucket.org/sinbad/ogre/src/4 ... .h?at=v2-1

Questions:

1. Am I correct that the OgreExport macro is not necessary in this file? I see only header-only/inline code.
2. In the transition document it says:
"Note that, for example, Entities are completely different from SceneNodes (they don't even share a common base class), so it is possible for an Entity and a SceneNode to have the same Id."
Wouldn't making IdObject template help with type-safety and value matching?
I mean something like:

Code: Select all

// do NOT modify IdType.
//...

template< class  AssociatedType >
class IdObject
{
 // ... same as before, do not use AssociatedType at all
};

// ...
// then provide actual types, maybe pre-instantiated in a cpp somewhere, or not:
class SceneNode; // no definition needed, the type is never used
class Entity; // no definition needed, the type is never used

// Use these types in the Ogre API
typedef IdObject<SceneNode> SceneNodeId; // C++11 would be:  using SceneNodeId = IdObject<SceneNode>;
typedef IdObject<Entity> EntityId; // C++11 would be:  using EntityId = IdObject<Entity>;

//...
It looks like a simple change that would avoid some category of runtime errors (trying to search a scene node using the id of an entity).
I use a very similar technique in several projects and it works very well as long as you keep a comon id value (here IdType).
The cost would be minor: more types would be defined but they would disappear in the optimized builds anyway (compilers optimize away this kind of case by having only one implementation for type several names).
Also, in my projects so far adding the template to id types this way didn't impact build times. (I did it atomically, once per different project, to be sure to see the impact in different use cases)

I can make a pull request if you think it's would help to do it, or if you just want to check yourself if it's cool.
The main issue would be replacing manually IdObject usage by more specific versions of it, but it should be obvious to do for me I suppose.


3. Header guards don't have any kind of prefix and use reserved __-prefixed named. Are there plans to fix that later? Want any help with cleaning?
User avatar
dark_sylinc
OGRE Team Member
OGRE Team Member
Posts: 5292
Joined: Sat Jul 21, 2007 4:55 pm
Location: Buenos Aires, Argentina
x 1278
Contact:

Re: [2.1] ObjectId small improvements

Post by dark_sylinc »

Klaim wrote: 1. Am I correct that the OgreExport macro is not necessary in this file? I see only header-only/inline code.
I can't say for certain. C++ is a complex language. You can remove it, it works, and you find several weeks later it breaks the build when using feature X on OS Z.

I think I recall adding it because some compiler was complaining. But I'm not sure now.
Klaim wrote: 2. In the transition document it says:
"Note that, for example, Entities are completely different from SceneNodes (they don't even share a common base class), so it is possible for an Entity and a SceneNode to have the same Id."
Wouldn't making IdObject template help with type-safety and value matching?
I mean something like:
That won't fix anything because we do not always reference the IdObject class, but rather the IdType (which is just an integer) that returns the function getId().
The ID is used for efficient sorting internally in the engine, and doing some look ups & insertion/removals. Using IdObject<type> instead of IdType would add an extra indirection in a performance critical place. We would also lose the ability to control the size of key (becomes 64-bit in x64 due to 64-bit pointers).

Ogre also doesn't directly expose IdObject to make those mixes. For example SceneManager::destroySceneNode and destroyEntity take a SceneNode & an Entity pointer respecitvely. The type safety is still there, which makes my next point:

If the user wants to use IdObject for himself, then if he wants type safety he should go for the minimum base class that makes sense (MovableObject for Entity, Node for SceneNodes, Renderable for SubEntity) rather than using IdObject which will put a lot of completely unrelated objects into the same bag.
Which is also what we do in Ogre as well.

Adding a template to add an extra layer of type safety is "morally" wrong (because you shouldn't be looking at that level, the only reason for that class is to reuse code from having shared functionality; you should look at higher levels like MovableObject or Node), bloats the code, bloats the executable size (internally the compiler will have to add more code for identical functionality; this rarely ever gets merge-optimized for very specific reasons), implies adding a level of indirection we do not want or need, makes us lose control over the size of the key, for no real benefit.

So, answer is no.
Klaim wrote:3. Header guards don't have any kind of prefix and use reserved __-prefixed named. Are there plans to fix that later? Want any help with cleaning?
I'm slowly fixing some headers when I see one and I'm working on it. My current convention is _OgreNameOfClass_H_.
Frankly it has not been a high priority for me since some users can be very pedantic about it, but the double underscore is reserved to prevent name clashing in the future with language or compiler extensions, which is pretty rare given our names are extremely long and specific.
"Morally" we shouldn't use them, but I'm not too concerned. If you want to mass changed them it's ok. Just watch out for the right EOL and to use spaces instead of tabs which is the common problem when seeing PRs with mass changes.
Post Reply