Jump to content
  • Advertisement
Sign in to follow this  
Chicktopus

Loss of pointer data when copying

This topic is 1339 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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!

 

Share this post


Link to post
Share on other sites
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.  :-)

Share this post


Link to post
Share on other sites

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.

Edited by Zipster

Share this post


Link to post
Share on other sites
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. Edited by Washu

Share this post


Link to post
Share on other sites

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);
  }
}

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

 

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.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!