Hi everybody,
Let's take the example of one keyframe class and one track class.
The track contains an array of keyframe, so the track own the keyframe.
Is it a bad practice to get the pointer of the track, the owner, in keyframe and remove from the track in the destructor?
Basically doing "delete keyframe", you don't have to do remove from the track before the delete.
But then the issue of this design is if multiple class share the element, it's not safe because it will remove only on the track but not the other place.
Another issue is if you create the keyframe on the stack and add this keyframe in the track:

{
  Keyframe keyframe;
  m_Track.Add(keyframe);
}

At the end of this scope, the keyframe will be removed from the m_track, it's not safe on this case too.
What do you think about all that?

It's not really bad practice, in fact it's quite common.  To do something like this in C++ you will typically use smart pointers.  To support single ownership you can use std:unique_ptr.  To support multiple ownership you can use std::shared_ptr.  Alternatively you can write your own smart pointer system or implement a garbage collector but these options are less often taken.

I am not really sure how you are doing things, but it sounds to me like you are doing memory management yourself with new and delete. From what I understand, I think your class layout is suboptimal. Please provide a little bit more code, so that I could get a better understanding of what you are doing.

1 hour ago, Alundra said:

Is it a bad practice to get the pointer of the track, the owner, in keyframe and remove from the track in the destructor?

It is definitely sub-optimal for various reasons. You listed some of them yourself.

As @Gnollrunner mentioned, in C++ you should think about using smart pointers and STL/Boost/etc.-containers to handle memory management. They exist to avoid the problems you mentioned.

50 minutes ago, Alundra said:

But then the issue of this design is if multiple class share the element, it's not safe because it will remove only on the track but not the other place.

You can use shared_ptr and weak_ptr in this case. Weak_ptr help you to avoid problems when the keyframe is deleted. Before you use the object it is pointing to, you have to check if it is still valid. Of cause, this introduces an overhead to your program. Additionally, shared ownership introduces a whole bunch of other issues, that might haunt you later.

To my experience, situations like you described result from suboptimal and too complicated class layouts - not always, but often. Maybe you should check if the way you are doing things is really the optimal solution to your problem. Why does another class need direct access to a specific keyframe? Can your track class provide interfaces to do certain operations on a keyframe instead of exposing them to the outside?

1 hour ago, Alundra said:

Another issue is if you create the keyframe on the stack and add this keyframe in the track:

{
  Keyframe keyframe;
  m_Track.Add(keyframe);
}

At the end of this scope, the keyframe will be removed from the m_track, it's not safe on this case too.

Again, a problem of your layout. If your object deletes itself from its owner then nobody else but the owner should be able to create such a class. What happens if you create a keyframe and don't add it to the track? What is deleted from the track in this case? So m_Track.Add(...) should take care of the construction. You can use a private ctor for Keyframe and give friend access to the track class to restrict construction outside of a track. Not the best solution, but works.

I think the best solution for you would be to put some deeper thoughts into your ownership relations.

In case I misunderstood you, forget about all the crap I wrote. ?

Greetings

4 hours ago, Alundra said:

Is it a bad practice to get the pointer of the track, the owner, in keyframe and remove from the track in the destructor?

Nope, this is C++, where you are in full control of what happens.

The language has lots of things to assist you in avoiding programming mistakes. Nobody is however forcing that on you though, and if you choose not to use them, that's fine by the language (ie you are in full control).

The other side of the coin is of course that you are also responsible for not messing up. The language is not coming to rescue you from yourself. If that bothers you, use a different design.

I don't like your design, because you are creating a circular dependency between track and keyframe, adding O(N) useless copies of the track pointer.

It is better if the keyframe never needs to know that there exists a track.

Here the code explaining:

Keyframe::Keyframe()
: m_Owner(nullptr)
{}

Keyframe::~Keyframe()
{
  if (m_Owner)
    m_Owner->RemoveKeyframe(this);
}

void Keyframe::SetOwner(Keytrack* keytrack)
{
  m_Owner = keytrack;
}

void Keytrack::AddKeyframe(Keyframe* keyframe)
{
  keyframe->SetOwner(this);
  m_KeyframeArray.Add(keyframe);
}

16 minutes ago, Alundra said:

Here the code explaining:

Keyframe::Keyframe()
: m_Owner(nullptr)
{}

Keyframe::~Keyframe()
{
  if (m_Owner)
    m_Owner->RemoveKeyframe(this);
}

void Keyframe::SetOwner(Keytrack* keytrack)
{
  m_Owner = keytrack;
}

void Keytrack::AddKeyframe(Keyframe* keyframe)
{
  keyframe->SetOwner(this);
  m_KeyframeArray.Add(keyframe);
}

 

This isn't the way I would do it. Again, I would represent ownership with smart pointers.  For instance if Keyframes are allowed in more than one Keytrack,  then for example each Keytrack could have a list shared_pinters to Keyframes. 

Or perhaps a Keyframe can only go in one Keytrack at a time. In that case I might make a doubley linked list as part of the Keyframe object itself (I often use multiple inheritance for this, if my object already inherits from something else), and then you could make the keyframe able to remove itself from the Keytrack.

I use a system like this. I call it a "group". An object can only be in a single group at a time and when you insert it into that group it increases the reference count. You can also have other normal smart pointers to that object.  If you remove it from the group and it still has other pointers to it, it sticks around. Otherwise it's reference goes to zero and it's collected. I have a few kinds of objects that work like this.

Anyway there are a lot of ways of doing these kinds of things.  Some people like weak pointers for instance. It's often a matter of preference.

Games, like all complex programs, absolutely must consider object lifetimes.  Objects need to be created at well-specified times, live from one designated time to another designated time, and then be destroyed and cleaned up at the end of their lives.

Whatever plan you follow, you need to follow it consistently. That seems to be the problem here.

Historically there have been many restrictions on memory, because game consoles had very little memory available, and a failed allocation due to many reasons (like memory fragmentation) generally results in program-terminating behavior.  This in turn led to various practices. Some only allow allocations/released during level transitions or loading screens. Some have required no allocations/releases at all, only loading the entire world as a single block, then unloading the entire world as a single block.  Some games have limited allocations/releases to only be done outside of certain times, like outside of the main loop during inter-frame delays.

Many object lifetimes must be guaranteed by policies.  When a function receives an object pointer as a parameter, a general guarantee is that the object will remain in memory for the duration of the function. However, unless it is specified in the broad design saving the pointer to the object is bad because the pointer may be a temporary object, or may be unloaded through other systems.  You may decide that these pointers are no longer guaranteed at the end of a graphics frame, or that they are no longer guaranteed after a level load, or that the code must always follow a pair of registering an object exactly once, then unregistering the object a single time before it is destroyed.

Smart pointers can solve a few problems related to object management when there are multiple systems that must share certain responsibilities for object lifetimes, but they do not replace a solid design.  Smart pointers still fail terribly under many conditions, and they still suffer from resource leaks under conditions like circular references.  They do not replace the fundamenal issue that object lifetime must be well understood.

Getting back to the issue with this code...

Under one set of circumstances, the class is using an external object. Other systems own the lifetime of the object, other systems create the object, and other systems destroy the object.  This is the most common pattern in software.

Under another set of circumstances, the class acquires ownership of an external object.  Other systems create the object, but under various circumstances this system will destroy the object.  This is NOT the typical pattern in software.  The typical object lifetime rules are being violated.

In general you should write code that follows a source/sink model.  If a system is the source of creating objects, it should also sink or destroy them when they are done. That is, you should always have a pair of functions for create/destroy, allocate/free, new/delete, load/unload, add/remove, and keep those responsibilities paired together.  Either this object controls ALL aspects of sub-object lifetime, or it controls NONE of the sub-object lifetime.

With an 'all or none' guarantee, code that uses the system can follow more broad policy rules about when it is safe to allocate or release resources. 

This also allows for alternate forms of memory management.  You can use different systems for pools of memory, allocating some stuff from one pool or global memory, allocating other stuff from different pools or different memory managers.  If you do ALL the work you can use your specified pool or memory manager. If you do NONE of the work those who use the system can use whatever memory management they wish. But if you take partial control, outside systems are limited to only use the forms you use in your half of the system.

To me, language has very little to do with this but rather this sort of hierarchy becomes a facet of the single responsibility principle. The keyframe should do what its name implies - store information about some properties. It is owned by the track, whose responsibility is to store some duration and a number of keyframes. Hence the track should remove or add keyframes. Copying keyframes from one track to another should be done by whatever structure you have that manages the tracks. If your keyframe has access to/knowledge of the track, the two become interdependent, which unnecessarily complicates the entire architecture.