Loss of pointer data when copying

Started by
7 comments, last by Pink Horror 9 years, 7 months ago

Hi guys

I'm having some trouble deep copying data. When the function in which I do the copying returns, the data is maintained, however when the functioned which called the function to copy the data returns, part of the copied data is immediately lost. In the code below, the aiAnimation* pushed on to the vector is not lost, but its aiAnimation->mChannels value is. The aiAnimation->mChannels property is type aiNodeAnim** (a pointer to an array of aiNodeAnim) .

It is worth noting that the Assimp::Importer cleans up after itself when it goes out of scope at the end of the function.

Here's the function which copies the data:



void Animation::AddAnimation(const char* p_filePath)
{
  Assimp::Importer m_importer;
  const aiScene* m_scene = m_importer.ReadFile(p_filePath,
  aiProcess_CalcTangentSpace |
  aiProcess_Triangulate |
  aiProcess_JoinIdenticalVertices |
  aiProcess_SortByPType | aiProcess_GenSmoothNormals);

  aiNodeAnim* m_channels = new aiNodeAnim[m_scene->mAnimations[0]->mNumChannels];
  for (int i = 0; i < m_scene->mNumAnimations; ++i)
  {
    aiAnimation* m_newAnimation = new aiAnimation(*m_scene->mAnimations[i]);
    for (int j = 0; j < m_scene->mNumAnimations; ++j)
    {
      aiNodeAnim* m_channel = new aiNodeAnim();
      m_channel = m_scene->mAnimations[i]->mChannels[j];
      m_channels[j] = *m_channel;
    }
    m_newAnimation->mChannels = &m_channels;
    g_animations.push_back(&*m_newAnimation);
  }
}

I've got a feeling there's something obvious I'm missing, but I'm kind of at a loss!

Advertisement

      aiNodeAnim* m_channel = new aiNodeAnim();
      m_channel = m_scene->mAnimations[i]->mChannels[j];

Just a guess, but aren't you overwriting the new m_channel pointer with a pointer to the data you're copying on the next line?

Unless there is some funky operator overloading going on, of course. :-)

I think, therefore I am. I think? - "George Carlin"
My Website: Indie Game Programming

My Twitter: https://twitter.com/indieprogram

My Book: http://amzn.com/1305076532

m_channels itself is a stack variable, to which you're storing the address in mChannels. So when the function goes out of scope, the address is no longer valid, and you also leak the memory.

So many things wrong here...
aiNodeAnim* m_channels = new aiNodeAnim[m_scene->mAnimations[0]->mNumChannels];
Assuming that all animations have the same number of channels...

for (int j = 0; j < m_scene->mNumAnimations; ++j)
Iterating over the number of animations, when what you want is the number of channels FOR an animation, sounds like a great chance to go past the end of the container and produce wonderfully well behaved undefined behavior cake.

aiNodeAnim* m_channel = new aiNodeAnim();
m_channel = m_scene->mAnimations[i]->mChannels[j];
Allocating memory then immediately overwriting that pointer with another pointer, thereby leaking memory. Furthermore, accessing a channel based on j, which is from the number of animations... not the number of channels IN an animation.

m_channels[j] = *m_channel;
Dereferencing the pointer and stuffing it into an array, why did we allocate this in the first place? Using j, again.

m_newAnimation->mChannels = &m_channels;
Taking the address of a stack allocated temporary and storing it. Always a bug.

g_animations.push_back(&*m_newAnimation);
So... we're dereferencing a pointer and then taking the address of the derefenced object and storing that into a container. Or you could just push back the pointer.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

Thanks for the quick response guys. I'd just re-written that code so it looks like I'd missed some bits!

I've changed the code based on your suggestions but leaving the function creates an access violation so I've obviously done something wrong! The function now looks like this:


void Animation::AddAnimation(const char* p_filePath)
{
Assimp::Importer m_importer;
const aiScene* m_scene = m_importer.ReadFile(p_filePath,
aiProcess_CalcTangentSpace |
aiProcess_Triangulate |
aiProcess_JoinIdenticalVertices |
aiProcess_SortByPType | aiProcess_GenSmoothNormals);


for (int i = 0; i < m_scene->mNumAnimations; ++i)
{
  aiAnimation* m_newAnimation = new aiAnimation(*m_scene->mAnimations[i]);
  for (int j = 0; j < m_scene->mAnimations[i]->mNumChannels; ++j)
  {
    m_newAnimation->mChannels[j] = new aiNodeAnim((const aiNodeAnim&)*m_scene->mAnimations[i]->mChannels[j]);
  }
  g_animations.push_back(m_newAnimation);
  }
}

m_newAnimation->mChannels[j] = new aiNodeAnim((const aiNodeAnim&)*m_scene->mAnimations->mChannels[j]);

Why are you doing this cast? If the compiler was complaining, that's probably a sign that this isn't valid, not that you should cast it and make it (pretend) to be valid.

aiAnimation* m_newAnimation = new aiAnimation(*m_scene->mAnimations);

Two things:

First, your variable naming is weird and confusing. The m_ prefix is more often used for member variables in a class and not for local variables. I'm not sure if that p_ in your parameter is for "parameter" or because it's a "pointer." There's no single standard or mandate you have to follow and you can name them however you want but sticking to something like Google's or LLVM's standard at least helps random newcomers to your code understand it faster.

Second, in clean and modern C++ you should (almost) never ever use `new` or `delete` but rather use owning smart pointers (e.g. std::unique_ptr<>) and then use smart pointer factories (e.g. std::make_unique<>) to allocate and initialize an object. This will greatly reduce the number of bugs you have. Use raw pointers for non-owning nullable references. Some old APIs like AssImp predate these conventions so there can be some wrangling involved to pass ownership around here, but it's not that bad and it still reduces bugs when used consistently.

leaving the function creates an access violation


It always amazes me when engineers think it's ok to give a vague description of an error. smile.png What is the _exact_ error message you're getting? On exactly which line is the debugger indicating the error occurs on? Pretend we're engineers having to debug a problem from the information given to us and need as much of it as possible, only don't pretend. smile.png

This may likely have a lot to do with how you're trying to make a copy of the AssImp animation. You're doing a deep copy of some of the channel data, but what about other data? You're copying mChannels, but what about mMeshChannels? What about the keys in those channels (mPositionKeys, mRotationKeys, mScalingKeys)? You need to deep copy _everything_. You're likely making some shallow copies of various pointers in both aiAnimation and aiNodeAnim.

One way to be sure you're getting everything - and bring about a number of other conceptual and runtime advantages - is to create your own animation data structure and file format optimized for your engine/renderer. AssImp should be used to import data from one of its supported "source" formats into your custom format. Any data you fail to copy is implicitly lost (and safely). Your custom structure can also make better use of modern C++ practices and avoid many of the headaches with AssImp's aging data structures.

Sean Middleditch – Game Systems Engineer – Join my team!

Apologies for any ambiguity, I've been coding in one language or another for almost 18 hours today, so the ol' noodle's a little fried!

My naming convention is 'g_' for variables global to a class, 'm_' for variables which are members of a function, 'p_' for parameters. Old habits and all that. I'd left out the mMeshChannels (among other bits) intentionally to keep the problem simple

I try to avoid using 'new' wherever possible (if I can let C++ deal with it, all the better), I've never actually had to use smart pointers before, but the bit of reading I did around them suggests that the data in them gets destroyed when the variable goes out of scope, which is why I stuck with 'new'. I take your point about the deep copy though, I should definitely look in to my own data format.

For reference, the exact error was "Unhandled exception at 0x77C7E3BE (ntdll.dll) in Engine.exe: 0xC0000005: Access violation reading location 0x305AE778.", which looking back at the shallow copy, is probably because something got cleaned up.

My naming convention is 'g_' for variables global to a class, 'm_' for variables which are members of a function, 'p_' for parameters. Old habits and all that. I'd left out the mMeshChannels (among other bits) intentionally to keep the problem simple


Not to nitpick too much, but... that's weird. Also, the terminology is all wrong. "Member" has a specific definition in C++ and local variables are _not_ members. Globals likewise are pretty universally considered to be a bit bigger than just "to a class." To each his own, but I'd argue that there's very real value to using more common conventions, especially when interfacing with other human beings.

I try to avoid using 'new' wherever possible (if I can let C++ deal with it, all the better),


That's good practice. That's usually called a preference for values rather than references.

I've never actually had to use smart pointers before, but the bit of reading I did around them suggests that the data in them gets destroyed when the variable goes out of scope, which is why I stuck with 'new'.


That's partly true. A smart pointer cleans up after itself. A modern smart pointer can easily deal with transfer of ownership, though (pre-C++11, the language made automatic transfer of ownership awkward and error-prone).

Consider:

std::unique_ptr<Foo> make_foo() {
  std::unique_ptr<Foo> local = std::make_unique<Foo>();

  // do stuff with `local`
  stuff(*local);

  // this implicitly transfers ownership from `local` to the caller via the return value
  return local;

  // `local` goes out of scope and is destroyed
  // however, it no longer owns the instance of Foo (the caller does) so that instance is _not_ destroyed!
}

void sink(std::unique_ptr<Foo> ptr) {
  // do stuff with `result`
  other_stuff(*ptr);

  // `ptr` is going out of scope and it owns the Foo object
  // so _now_ the Foo instance is destroyed! (we could save it off
  // to a member variable or give it to another function or
  // return it back to the caller, if we wanted to)
}

int main() {
  // acquires ownership from the callee
  std::unique_ptr<Foo> result = make_foo();

  // do stuff with `result`
  other_other_stuff(*result);

  // transfer ownership to another function - must explicitly state that you're releasing ownership via std::move
  sink(std::move(result));

  // `result` goes out of scope and is destroyed
  // however, it no longer owns the instance of Foo (we gave it to `sink`) so the object is not double destroyed!
}
The smart pointer will only destroy its contained value when it semantically should. In the example above, the make_foo function cleanly returns an owned pointer. It is not accidentally deleted. It is not dangling. In fact, unique_ptr makes it impossible (well, moderately difficult) to have a dangling pointer or to forget to delete an object but makes it easy to pass around ownership of the object, which is exactly what you want. The only difference with a regular pointer is that you might have to call .get() or std::move() here and there when the compiler isn't sure how you want to affect ownership (and since you have to be explicit that is also good as now your code is easier to read and reason about!).

You can have std::unique_ptr as a local variable, a return value, a parameter, a type member, a global, you can put it in a std::vector or in any other standard container, etc. In the vast majority of cases, it Just Works(tm) with no surprises.

Use C++ references (`Foo&` or `Foo const&`) when you want a non-nullable non-owning reference to an object. Use raw pointers (`Foo*` or `Foo const*`) when you want a nullable non-owning reference to an object. Use a unique pointer (`std::unique_ptr<Foo>`) when you want a nullable owning reference to an object. Use a regular value (`Foo`) when you want a non-nullable non-owning copy of an object. There's a few other smart pointers in the standard for more specialized cases and you can of course write your own or find others in existing libraries for even more specialized cases.

std::unique_ptr has also been built to deal with older APIs and C APIs that require a special deleter (rather than just calling `delete`), e.g. for SDL2 objects I frequently use code like:

auto window = std::unique_ptr<SDL_Window, void(*)(SDL_Window*)>(SDL_CreateWindow(/*...*/), &SDL_DestroyWindow);
Unfortunately, make_unique doesn't handle deleters resulting in the overly verbose ugly syntax above. You can write a helper like this, though:

// so you don't have to spell out the deleter's type when passing parameters or the like
template <typename T>
using scoped_resource = std::unique_ptr<T, void(*)(T*)>;

// acquire ownership of a pointer and assigns the specified deleter
template <typename T, typename D>
scoped_resource<T> make_scoped(T* ptr, D&& deleter) {
  return {ptr, std::forward<D>(deleter)};
}

// example
auto window = make_scoped(SDL_CreateWindow(/*...*/), &SDL_DestroyWindow);
So now you have a unique_ptr that references a window and will automatically call the right function when/if it is responsible for deleting the object. It's not quite the perfect interface which is why a distinct scoped_resource has been proposed for standardization a few times, but it works surprisingly well.

... and wow did I just get off topic.

Sean Middleditch – Game Systems Engineer – Join my team!

Thanks for the quick response guys. I'd just re-written that code so it looks like I'd missed some bits!

I've changed the code based on your suggestions but leaving the function creates an access violation so I've obviously done something wrong! The function now looks like this:


void Animation::AddAnimation(const char* p_filePath)
{
Assimp::Importer m_importer;
const aiScene* m_scene = m_importer.ReadFile(p_filePath,
aiProcess_CalcTangentSpace |
aiProcess_Triangulate |
aiProcess_JoinIdenticalVertices |
aiProcess_SortByPType | aiProcess_GenSmoothNormals);


for (int i = 0; i < m_scene->mNumAnimations; ++i)
{
  aiAnimation* m_newAnimation = new aiAnimation(*m_scene->mAnimations[i]);
  for (int j = 0; j < m_scene->mAnimations[i]->mNumChannels; ++j)
  {
    m_newAnimation->mChannels[j] = new aiNodeAnim((const aiNodeAnim&)*m_scene->mAnimations[i]->mChannels[j]);
  }
  g_animations.push_back(m_newAnimation);
  }
}

I'd have to see the definition of all of these types in order to say anything meaningful. For example, what's mChannels? What does the copy constructor for aiAnimation look like?

From what I understand, you're trying to construct a temporary scene from a file, then deep-copy the animations out of the scene, and then destroy the scene. Why aren't you taking ownership of the animations out of the scene, instead of copying them? Then you don't need a deep copy.

This topic is closed to new replies.

Advertisement