RAII for scene objects (entities, billboard sets, etc)

[cdleonard]

Right now there is no unified way to destroy an object in Ogre. The "C++ way" to destroy an object is to call delete on it, but this generally does not work in Ogre (it corrupts *Manager classes).

Objects which are created from a scene manager must be destroyed by the same scene manager (with the correct method). If you createCamera you must remember to use destroyCamera(), etc.

I tried to destroy a BillboardSet using _getCreator()->destroyInstance(bbset) but it seems to corrupt memory and throw an access violation on shutdown.

What I want is a way to automate memory management of scene objects using standard C++ semantics. By scene object I mean more or less everything derived from MovableObject.

I know that Ogre::SceneManager takes ownership of all such objects; but I don't need that. I want things like ParticleSystemPtr and EntityPtr with the same semantics as std::auto_ptr. I can then use them as stack variables and private members in my own entity classes; without having to write destructors by hand.

I can make my own but it's not very pretty; since each of those classes has to be destroyed in it's own way. I think that this functionality is useful and belongs inside Ogre.

[sinbad]

We already have this on the TODO, it will get done when we have the time.

[yeahRIGHT]

Just found this old thread; how will this look like in the future? What I would like to have is that it is legal to call delete on a scene object, so that I can store it in an auto_ptr. I often have to write destructors like:

Code: Select all

GameSceneManager::~GameSceneManager() {
		scene.getRootSceneNode()->detachObject(selectionFrame);
		scene.destroyManualObject(selectionFrame);
}

With RAII, no destructor would be required. The destructor of the scene object could to the unregistration and AFAIK the operator "operator delete" can be overloaded so that the correct deallocation takes place.

The other solution having EntityPtr, ParticleSystemPtr, etc. for every type is not as beautifull imho.

[sinbad]

auto_ptr is horrid, it just passes ownership on assignment which is no protection against deleting at the wrong time.

boost::shared_ptr is far superior which is what our SharedPtr is based on (without needing the Boost dependency). We still intend to implement this but haven't got to it yet. I don't know why you don't consider it 'as beautiful' since auto_ptr can lead to trailing garbage pointers whilst shared_ptr cannot (since its reference counted). All auto_ptr guarantees is that the object will get deleted, but that can easily leave dangling pointers in other classes. Not at all appropriate for the shared object setup being proposed. It's fine for simple ownership situations where everything else uses raw pointers, but that's it.

[cdleonard]

With RAII, no destructor would be required. The destructor of the scene object could to the unregistration and AFAIK the operator "operator delete" can be overloaded so that the correct deallocation takes place.

The other solution having EntityPtr, ParticleSystemPtr, etc. for every type is not as beautifull imho.

Overriding operator delete is complicated and painful. Doing this in the MovablObject destructor is also somewhat painful. You eventually have to distinguish between calling "delete pManualObject" externally and from inside SceneManager::destroyManualObject.

auto_ptr is horrid, it just passes ownership on assignment which is no protection against deleting at the wrong time.

boost::shared_ptr is far superior which is what our SharedPtr is based on (without needing the Boost dependency). We still intend to implement this but haven't got to it yet. I don't know why you don't consider it 'as beautiful' since auto_ptr can lead to trailing garbage pointers whilst shared_ptr cannot (since its reference counted). All auto_ptr guarantees is that the object will get deleted, but that can easily leave dangling pointers in other classes. Not at all appropriate for the shared object setup being proposed. It's fine for simple ownership situations where everything else uses raw pointers, but that's it.

I don't understand why people claim shared_ptr is better than auto_ptr. They're different tools with different semantics for different purposes. It is indeed unfortunate that standard C++ only includes auto_ptr; but that's fixed by tr1::shared_ptr.

I'm not sure what "shared object setup" you're proposing; would you care to elaborate?

Auto_ptr will destroy the object when the owner goes out of scope. SharedPtr will destroy the object when the last reference is gone; and does not really work with "registries" like SceneManager or ResourceManager; which keep references forever. There's more to destruction than avoiding leaks and crashes.

There are some situations where you want to explictly destroy/remove an object. Let's say you have a custom object which controls an Ogre::Entity (a high-level car object). When that custom object is destroyed the entity should be explictly removed from the scene; and that currently requires an explicit call in the destructor to destroyEntity. I don't see how holding SharedPtrs to Entities in SceneManager would fix this; you would still need to tell the scene manager to remove the entity from the scene.

A clearer example is material cloning: Sometimes you want to clone a material and modify it for a specific object (let's say you need a specific pattern of stripes on your car). When you're done with the clone you need to explicitly remove the MaterialPtr from the MaterialManager. If you don't then you get a memory leak of one unused material referenced by the MaterialManager. The current solution is to again write code in your own destructor. It would be a good idea to have a special kind of pointer to cloned materials; which you can keep as a member in the hypothetical "Car" class.

Writing custom destructor code is far from ideal; it can be incorrect in subtle ways. It's better to always avoid it. Imagine we have a Car class which creates an entity and clones a material in the constructor; and then releases both in the destructor. This is not exception-safe: if an exception is thrown from cloning the material then Car::~Car is not called and the entity is leaked into the scene. Even if you catch the material exception higher up you still get a car in the scene; with the default material. You can fix this by having smart pointer members with auto_ptr semantics; since members are still destroyed on constructor exceptions. Otherwise you need to try/catch inside the constructor; and nobody does that. Smart pointers are correct by default.

Full C++ exception safety is very hard and most people don't bother with it. Still; it's nice to provide the appropriate tools inside a library like Ogre. Custom destructors should never be required!

Since I first posted this feature request (10 months ago) I implemented this inside Caelum as Caelum::OwnedPtr. It's documented; tested and covers the above cases (including cloned materials). I'd like to propose it for inclusion inside OgreMain. As far as I can tell the only changes required are replacing "Caelum" with "Ogre" and the license header. If you're interested I can submit it as a patch on the tracker.

[Speedo]

I don't understand why people claim shared_ptr is better than auto_ptr. They're different tools with different semantics for different purposes.

In general terms I'd call it superior mainly for shared_ptr's ability to take deallocators. They allow it to easily handle types that auto_ptr simply can't deal with.

[sinbad]

The problem I generally have with with auto_ptr is that it insists on a single owner. It's therefore unsuited for instances where ownership is shared - auto_ptr is, in design terms, only useful for composition / aggregation structures. So your clone example works because you have a very simple, strong ownership case. The vast majority of cases aren't like that with resources.

I agree the 'registry' keeping a reference can sometimes be a pain. However, a lot of the time it's precisely what you want for resources - for example, just because you're not using a Mesh right now (say for a handful of frames or even a few minutes) does not for a second mean you want to destroy it. It's a shared resource, and the cost of recreating / reloading it next time you need it is prohibitive - the very last thing you want is for these things to be thrashing in and out of memory all the time based on unbatched, real-time demands. It makes far more sense to control the removal of these shared resources explicitly, not implicitly. However, it is nice to make sure that even when the user indicates that the resource is no longer of use, that the final destruction only happens when all references are lost - removing a lot of corner case ordering issues.

If you genuinely have a case where you want things to be destroyed immediately while still having a central reference, IMO shared_ptr/weak_ptr is a nice way of doing it.

I wasn't saying auto_ptr is useless, but it is extremely specific to strong single-ownership cases, of which there are really none in the (public) Ogre resource system. When it comes to scene objects, what's being talked about here is the ability for these objects to transfer more easily between SceneManagers, and to make them easier to pool and re-use. It's possible that an auto_ptr style ownership could be assigned to the parent nodes, but this would then prevent pooling of these resources (except also by auto_ptr, which then becomes pointless), and more importantly double-referencing is tricky when you don't know who will be the 'final' owner during a transition (this is important at boundaries in things like PCZ, where an object is temporarily owned by 2 zones at once, and you don't know which one it will eventually be in).

In short, I don't think auto_ptr is a good solution the cases being referenced here - the ownership is not as simple as is assumed, nor in most cases is an immediate unload / destroy model desirable because of the efficiency issues. You can cite a few specific cases where such a model would be useful, but there are far more counter examples.

[yeahRIGHT]

Consider the following class:

Code: Select all

class AxesDebugView {
public:
	AxesDebugView(Ogre::SceneManager& scene);
	~AxesDebugView();
	
	const Ogre::Vector3& GetPosition() const;
	void SetPosition(const Ogre::Vector3&) const;

private:
	Ogre::SceneManager& scene;
	Ogre::SceneNode* const gizmoNode;
	Ogre::Entity* const gizmo;
}

I think it's clear what the constructor and destructor are doing here. The constructor creates the node and the entity and the destructor removes and deletes them. The point is, if deleting SceneNodes and Entities would remove them from the scene, I could use the C++ RAII feature and wouldn't need to write a destructor at all. Having auto_ptr as members means (1) no problem with exceptions occuring in the constructor, leaving a leak behind (2) no need to write a destructor and forget to remove and delete anything.

Also it's clear, that no shared_ptr is needed here. The axes thing should exist in the scene until I choose to remove it, simply deleting the AxesDebugView object. The scene objects are not shared here; I know in my code, when I don't need a shared_ptr; OGRE can't know that. But it's not possible to choose my own pointer type because simply deleting the objects would cause some serious problems.

Since OGRE can't know what pointer type is right for my case, it shouldn't define which to use.

[yeahRIGHT]

Overriding operator delete is complicated and painful. Doing this in the MovablObject destructor is also somewhat painful. You eventually have to distinguish between calling "delete pManualObject" externally and from inside SceneManager::destroyManualObject.

Obviously, SceneManager::destroyManualObject() needs to remove the object from the graph and delete it; after the change I propose, it would only call delete obj; and the destructor would call the code that removes the object from the scene graph. It's no more code at all.

[sinbad]

Ok, I'll consider what you've said here when I come to tackle this. I still think that in practice you wouldn't want to implement something like this in a literal sense, because pooling objects is a much better idea than destroying them and recreating them all the time, particularly as dynamic worlds involve a lot of construction / destruction of nodes and objects which gets expensive quickly. I'm thinking perhaps that we could hide the 'real' implementation of things like the nodes behind a structure that you can delete, but which returns the real object to the pool for re-use, or something like that. And any real destructors would still have to do the same kind of de-registration process, since SceneManagers need to keep track of these objects, and that'll only get more complex as paging and zone systems are promoted up the interface hierarchy. Still, I guess all you're asking for is for that to be out of your way, even if it still happens in the background.

[imtrobin]

I think Ogre should not manage an internal pool like that. This will cause it difficult to calculate usage, important when going to consoles. These pooling should be left up to application.

Regarding RAII, what would happen if there are multiple scenemanagers or scenegraphs? Or if I have a external spatial database I do keep track of the entity too? I think how the dtor is called should be left up to application.

[sinbad]

I think Ogre should not manage an internal pool like that. This will cause it difficult to calculate usage, important when going to consoles. These pooling should be left up to application.

Not everyone seems to agree on this - often when we suggest to people that they should be pooling their objects instead of doing a ton of construction / destruction, they reply that they expected Ogre to do that for them. I don't see the harm in providing the option to do it.

Regarding RAII, what would happen if there are multiple scenemanagers or scenegraphs? Or if I have a external spatial database I do keep track of the entity too? I think how the dtor is called should be left up to application.

Well, this is connected with the whole reason for changing how nodes and objects are managed, and why RAll isn't as simple as might be perceived here. SceneManagers must know which objects they should be concerned with at any one time, but this ownership cannot be as tight as it has been so far, since ownership has to pass more easily between them - PCZ hit this problem and resolved it through its own intermediate objects. There will undoubtedly be a need for a set of backing objects which manage the association between nodes / objects and the SceneManager currently responsible for organising them in its spatial system, but there may have to be a fire-break wherein the internal implementations (SM-specific node information etc) can be attached / detached from the 'portable' instance as responsibility changes. What this discussion has done is raise issues about how the 'portable' part might be managed.

[cdleonard]

This topic is very confusing. I don't see why this got derailed into a discussion of paging design.

Ogre uses global registers for various objects; including meshes; materials; scene nodes and entities. There are various very good reasons it does that; however sometimes it is useful for the user to explicitly control the lifetime of such an object. This currently requires explicit deletion and I believe it shouldn't be the case; Ogre should provide special smart pointers for such cases. This can be supported without requiring their use; you can implement a PrivateMaterialPtr on top of a regular SharedPtr. This is NOT a core design issue; it's an orthogonal feature for C++ exception-safety freaks.

On pooling: are you seriously suggesting applications limit their creation / destruction of entities and scene nodes? This is not reasonable. Ogre::Mesh is an expensive object loaded from disk; but creating an Ogre::Entity shouldn't really cost significantly more than memory allocation. This is probably already the case; except for the cost of adding to name-based maps. The same applies to SceneNodes: if creating nodes is expensive then the space-partitioning code should be optimized.

As for paging: This will cause some scene objects to be loaded and maybe owned by individual pages. It must still be possible to create custom entities for things like player characters and the like; objects whose lifetime is completely controlled by the user. Those will still be owned by the "master registry" in Ogre::SceneManager and perhaps referenced by the space partitionings of different zones. But createEntity and destroyEntity will still work as they do now and Ogre can provide a smart "PrivateEntityPtr" for exception-safety freaks to use in their high-level Player classes. This would be useful even if Ogre switches to passing SharedPtr<Entity> instead of Entity* by default.

[imtrobin]

Ogre uses global registers for various objects; including meshes; materials; scene nodes and entities. There are various very good reasons it does that; however sometimes it is useful for the user to explicitly control the lifetime of such an object. This currently requires explicit deletion and I believe it shouldn't be the case; Ogre should provide special smart pointers for such cases. This can be supported without requiring their use; you can implement a PrivateMaterialPtr on top of a regular SharedPtr. This is NOT a core design issue; it's an orthogonal feature for C++ exception-safety freaks.

I'm as much as a exception safe freak as you but think about how to implement such a safe pointer. Say for example, the Ogre Entity. You will need to store additional pointer to the scene manager per smartpointer...wait, how about multiple scenemanagers, .. let's it store a list of pointers. Consider the overhead of that smart pointer. On PC, it might not matter much but with Ogre moving to consoles, memory management must be taken carefully.

Personally I think MaterialSharedPtr should not exist. It complicates memory calculations with reference counting.

Ogre::Mesh is an expensive object loaded from disk; but creating an Ogre::Entity shouldn't really cost significantly more than memory allocation.

That depends on application. Say you create different monsters from an same entity but using vertex morph that can be runtime deformed (by physics), essentially you need to clone the mesh for each monster.

[cdleonard]

I'm as much as a exception safe freak as you but think about how to implement such a safe pointer. Say for example, the Ogre Entity. You will need to store additional pointer to the scene manager per smartpointer...wait, how about multiple scenemanagers, .. let's it store a list of pointers. Consider the overhead of that smart pointer. On PC, it might not matter much but with Ogre moving to consoles, memory management must be taken carefully.

Personally I think MaterialSharedPtr should not exist. It complicates memory calculations with reference counting.

Actually auto_ptr can have zero overhead (depending on how good your compiler's inlining is). An auto_ptr does no reference counting: it's a struct with a single member for the base pointer and no vtable. Memory layout shouldn't change compared to using the base pointer directly. All it does is change unwind semantics for better exception safety (it's sort of equivalent to adding lots of try/catch).

If you're worried about efficiency then making some of the name-based hashes optional would probably help more; especially with memory use. C++ strings have no interning mechanism; long names are actually duplicated several times in memory.

Ogre::Mesh is an expensive object loaded from disk; but creating an Ogre::Entity shouldn't really cost significantly more than memory allocation.

That depends on application. Say you create different monsters from an same entity but using vertex morph that can be runtime deformed (by physics), essentially you need to clone the mesh for each monster.

Then ogre should pool vertex buffers instead of asking the application to pool entities. AFAIK sinbad already implemented something like that for intermediate compositor textures.

[cdleonard]

I'm as much as a exception safe freak as you but think about how to implement such a safe pointer. Say for example, the Ogre Entity. You will need to store additional pointer to the scene manager per smartpointer...wait, how about multiple scenemanagers, .. let's it store a list of pointers. Consider the overhead of that smart pointer. On PC, it might not matter much but with Ogre moving to consoles, memory management must be taken carefully.

Personally I think MaterialSharedPtr should not exist. It complicates memory calculations with reference counting.

Actually auto_ptr can have zero overhead (depending on how good your compiler's inlining is). An auto_ptr does no reference counting: it's a struct with a single member for the base pointer and no vtable. Memory layout shouldn't change compared to using the base pointer directly. All it does is change unwind semantics for better exception safety (it's sort of equivalent to adding lots of try/catch).

If you're worried about efficiency then making some of the name-based hashes optional would probably help more; especially with memory use. C++ strings have no interning mechanism; long names are actually duplicated several times in memory.

Ogre::Mesh is an expensive object loaded from disk; but creating an Ogre::Entity shouldn't really cost significantly more than memory allocation.

That depends on application. Say you create different monsters from an same entity but using vertex morph that can be runtime deformed (by physics), essentially you need to clone the mesh for each monster.

Then ogre should pool vertex buffers instead of asking the application to pool entities. AFAIK sinbad already implemented something like that for intermediate compositor textures.

[imtrobin]

Actually auto_ptr can have zero overhead (depending on how good your compiler's inlining is).

You can't have an auto_ptr style with zero overhead if you have multiple sceneManagers. How would it know which instance of the sceneManager it will remove from?

Then ogre should pool vertex buffers instead of asking the application to pool entities. AFAIK sinbad already implemented something like that for intermediate compositor textures.

I think either way, pooling in such a case should be left to application. Yes it means more work for us programmers, but flexibility and speed are always orthogonal goals, just like C++

[Klaim]

If you're worried about efficiency then making some of the name-based hashes optional would probably help more; especially with memory use. C++ strings have no interning mechanism; long names are actually duplicated several times in memory.

That's already planned for Cthugha.

That discussion makes me think that the future unique_ptr might be a good solution but alas it's not available yet.

[cdleonard]

That discussion makes me think that the future unique_ptr might be a good solution but alas it's not available yet.

An Ogre::UniquePtr would be easy to implement; with the same semantics as boost::unique_ptr. The thing most people hate about auto_ptr seems to be ownership transfer on assignment. That is sometimes useful; but perhaps it would be nicer to only offer it as a distinct "transfer" function.

[Klaim]

AFAIK, a std::unique_ptr require r-value reference semantic to be available...

[sinbad]

On pooling: are you seriously suggesting applications limit their creation / destruction of entities and scene nodes? This is not reasonable. Ogre::Mesh is an expensive object loaded from disk; but creating an Ogre::Entity shouldn't really cost significantly more than memory allocation. This is probably already the case; except for the cost of adding to name-based maps. The same applies to SceneNodes: if creating nodes is expensive then the space-partitioning code should be optimized.

It's not 'expensive' in the grand scheme of things, and as it happens our default allocator is very efficient. But nothing is as efficient as not doing the allocation / deallocation at all, and if you want the best performance yes, you would re-use rather than destroy / recreate.

Your 'PrivateObjectPtr' approach is indeed what I would do right now if you wanted to have the RAll sort of behaviour, but that won't cut it when objects start being passed between SceneManagers more freely, since your implementation won't necessarily know whose scene graph they're currently in. That's why I was suggesting a core wrapper that is capable of managing this case is on the cards for the future anyway, as well as removing the need to name objects and hash those names if you'd prefer not to need to.

Then ogre should pool vertex buffers instead of asking the application to pool entities. AFAIK sinbad already implemented something like that for intermediate compositor textures.

We already pool vertex buffers for animation, and 3rd party apps can key into this by requesting software animated buffers to perform their own animation if necessary. We don't require anyone to pool entities, it's just something you might want to consider doing if you create / destroy a lot of them and are looking to reduce allocator traffic.

[yeahRIGHT]

But nothing is as efficient as not doing the allocation / deallocation at all, and if you want the best performance yes, you would re-use rather than destroy / recreate.

Yes and no. Pooling also requires some sort of bookkeeping, just the same as with allocation / deallocation. But it is not a standard way (in the sense of exception-safety freakery) to speed things up and thus often regarded as anti-pattern. You can save overhead if you could avoid some expensive construction work by re-using entities and nodes, however, if they can be completely different from the old ones, you may save nothing.

In this case, you're as good as with a custom allocator, which effectively pools the memory for objects of the same type; or is there more to pool than just the memory of the objects?