UserData Memory Leaking Problem
-
On 28/06/2013 at 11:23, xxxxxxxx wrote:
User Information:
Cinema 4D Version: 13
Platform: Windows ;
Language(s) : C++ ;---------
Hi,I'm getting memory leaks when I run my custom UD sorting method.
The method is used to look for UD entries that are out of order. Then it copies their values, deletes it, and re-creates them with a lower level ID#. So my UD entries are always sequential in order.
FYI: The reason I need them to stay in sequential order because I'm using the UD entries to drive a Gedialog layout. And it's crucial that they stay in sequential order with no jumps or gaps in their ID#s.The method is working fine. And doing what it's supposed to do. But I'm getting a memory leak from it. And I can't figure out why.
I think the leaks are coming from this line of code: BaseContainer data = *bc->GetClone(COPYFLAGS_0, NULL);The method code:
void udSequential() { for(LONG i=0; i<100; i++) { BaseDocument *doc = GetActiveDocument(); BaseObject *obj = doc->GetActiveObject(); if(!obj) return; BaseTag *tag = obj->GetTag(TAG_PLUGIN_ID); //My tag's plugin ID is: TAG_PLUGIN_ID if(!tag) return; DynamicDescription *ud = tag->GetDynamicDescription();//The master UD container GeAutoDynamicArray<LONG>order; //Will be used to store the UD item's levels and compare them DescID dscID = NULL; //Each UD item is a DescID type object const BaseContainer *bc; void *dscHnd = ud->BrowseInit(); //Initialize the Browse function while(ud->BrowseGetNext(dscHnd, &dscID, &bc)) //Loop through the UserData entries { LONG level = dscID[1].id; //Get the leve ID# for each UD item order.Push(level); //Add this level number to the array LONG arrSize = order.GetCount(); //Get the length of the array LONG last = order[arrSize-1]; //Get the last array element LONG previous = order[arrSize-2]; //Get the second to last array element LONG diff = last - previous; //Get the amount of the difference if(diff > 1) //If the levels are not sequential { BaseContainer data = *bc->GetClone(COPYFLAGS_0, NULL); //Copy the UD item's container GeData d; //Create a GeData container to hold the UD item's gizmo values tag->GetParameter(dscID, d, DESCFLAGS_GET_0); //Copy the UD item's data to the "d" variable String name = bc->GetString(DESC_NAME); //Copy the UD item's label LONG type = bc->GetLong(DESC_CUSTOMGUI); //Copy the UD item's Type LONG unit = bc->GetLong(DESC_UNIT); //Get the units setting(Real, Percent. etc..) ud->Remove(dscID); //Delete the UD item (Note: This doesn't remove the level info) dscID.PopId(); //Delete the UD item's level from the UD stack dscID.PushId(level - 1); //Add the UD back to the UD stack with a lower level ID# ud->Set(dscID, data, NULL); tag->SetParameter(DescID(dscID), GeData(d), DESCFLAGS_SET_0);//Paste the copied data to this new version with new level# } } ud->BrowseFree(dscHnd); //Free the memory used by the BrowseGetNext() function } }Every time the method executes I get a leak with these two names:
btree_container(some random number)
ge_container.cp(some random number)Any ideas why C4D is refusing to let go of the memory for this?
-ScottA
-
On 29/06/2013 at 03:46, xxxxxxxx wrote:
You're cloning the BaseContainer but it's never being freed. That's your leak.
Steve
-
On 29/06/2013 at 08:33, xxxxxxxx wrote:
Hi Steve,
That's what I was thinking too. But how do we free a const BaseContainer copy?
The only thing I see is this: dataPos->~BaseContainer();
But when I use it locks up C4D.
And if I try to put it in the plugin's destructor. It crashed C4D.The other odd thing is that the method I posted above does not seem to leak when it's used all by itself on it's own. When it's used in a stand alone usage situation all by itself.
But I'm also doing a couple of other UserData copying and deleting operations before calling that custom method. And for some reason, it's this combination that is causing memory leaks.This is the other UD copying I'm doing before calling that sequential order method:
//I'm doing this to get at this UD entry's values to copy them to another UD entry //Then I'm deleting this UD entry const BaseContainer *dataPos = ud->Find(pos); BaseContainer bcPos = *dataPos->GetClone(COPYFLAGS_0, NULL); //I'm doing this to get at this UD entry's values to copy them to another UD entry //Then I'm deleting this UD entry const BaseContainer *dataRot = ud->Find(rot); BaseContainer bcRot = *dataRot->GetClone(COPYFLAGS_0, NULL);Since I'm using copy in several places. I guess it's now become very important to understand how to handle the memory from these ->GetClone(), and ->Find() operations.
@Maxon.
I have some comments (rants actually) about the way you've set up your UserData system for us to use.
Please take this as constructive feedback.Why on earth are we users forced to handling such low level memory operations for something as commonly used as UserData?
Look at my code. Can you see how dangerous and overly complicated this whole thing is to copy, move, and delete UD entries?
Why on earth is there no built-in sorting and sequential ordering functions in the SDK for UD?
Why do we have roll our own using very dangerous low level pointers?
The way you have set up the browsing and copying with pointers is extremely complicated. And most of all it's dangerous!UserData is one of the most commonly used interfaces for users. If anything in the SDK should be set up with lots of pre-built options without needing to handle the low level memory stuff by hand...It's UserData.
IMHO. This section of the SDK needs some TLC. Because what I'm doing (fighting with memory) with pointers here is extremely dangerous. And way too low level for something as commonly used as UserData.
Something this commonly used should be simpler to use for the users. And have more built-in options.
Please consider adding ordering and copying functions to it so we don't need to worry about pointers and memory leaks. Because doing it by hand like I'm doing is very dangerous!!!
Thanks for listening.-ScottA
-
On 29/06/2013 at 09:44, xxxxxxxx wrote:
Unfortunately, Maxon is working with a standard of C++ that goes back at least a decade. No exception handling which makes use of STL very 'dangerous' if not fully handled in the try/throw/catch paradigm. They are only slowly implementing systemic memory handling such as garbage collection. But I think the main problem is in transitioning an archaic system to something more recent leaves those artifacts - sort of like Windows with its MS-DOS underpinnings and many issues. You will have to live with them, learn how to adapt to them, and get the best results while showing a grinding grin. Check every allocation and pointer. Be methodical in your logic and program flow. Hope to hell that the gremlins don't find you!
-
On 29/06/2013 at 10:37, xxxxxxxx wrote:
Yeah I agree.
Just about everything in the C4D C++ is fairly dangerous and requires a lot of low level memory handling. But if there's one area of the SDK that I expected it to be fully fleshed out and have lots of built-in functions. It would be the UserData section.
I'm rather shocked at how such an important part of the program has so little SDK code written for it. And how much low level memory management we have to handle ourselves.Anyway,
Back to the problem. I have to figure out how I can use multiple ->GetClone() lines of code in my plugins. And not get memory leaks.-ScottA
-
On 29/06/2013 at 10:53, xxxxxxxx wrote:
Unfortunately, Maxon is working with a standard of C++ that goes back at least a decade.
This is not true any more. C4D R14 SDK uses some C++11 features, especially in c4d_misc !!!
IMHO Exceptions in C++ are bad Idea in many case.
Yes of course STD containers require Exceptions and this is main problem with it.> And how much low level memory management we have to handle ourselves.
If you do it manually then it is dangerous and cumbersome, but you do not need to do this just use RAII all the time.
This is pretty easy if you can use C++11, but of course also possible in C++03 too.BaseContainer data = *bc->GetClone(COPYFLAGS_0, NULL); ///this is really bad idea!!! MemLeak !BaseContainer *data = bc->GetClone(COPYFLAGS_0, NULL); /// a little bit better, but still dangerous. ... a lot of code ... gDelete(data ); ///possible MemLeak !!!AutoPtr<BaseContainer> data( bc->GetClone(COPYFLAGS_0, NULL) ); ///Using RAII it is hard to make MemLeak.BaseContainer data; bc->CopyTo(&data,COPYFLAGS_0,NULL); /// here is another possilbe solutionOs course the question why do you need BaseContainer data at all if you do not use it in you code ???
-
On 29/06/2013 at 11:00, xxxxxxxx wrote:
Double post, sorry
-
On 29/06/2013 at 11:02, xxxxxxxx wrote:
On a side note: Is there a functionality in Cinema _somewhere_ that allows userdata
to get sorted? I don't know of any. Yeah, of course it would be useful if there was a
an SDK function that automatically does this, but you could list up plenty of other
methods that would be useful when being included directly in the SDK. Maxon can't just
put all functions that someone may need at some point. It would blow up the frame. Everything
in the SDK is what Maxon uses themselves. Any other functionality can be implemented
by third parties, which is basically the principle of plugins.Memory handling has always been necessary in C and C++. And the SDK is very precise
when it comes to information on when memory is yours or the applications memory.That's my opinion.
-Niklas -
On 29/06/2013 at 13:05, xxxxxxxx wrote:
Remo,
Thanks for your suggestions. I tried them but I had some troubles.
This is a section of the code block that I have in my plugin that copies one UD values to another UD://Get the data from the third UD entry DescID pos(DescLevel(ID_USERDATA, DTYPE_SUBCONTAINER, 0), 3); const BaseContainer *dataPos = ud->Find(pos); //Get the data for this specific UD item BaseContainer bcPos = *dataPos->GetClone(COPYFLAGS_0, NULL); //Get a copy of the UD's BaseContainer GeData dPos; tag->GetParameter(pos, dPos, DESCFLAGS_GET_0); //Get the values for the gizmos in the source UD we want to copy and store them in d String name = dataPos->GetString(DESC_NAME); //Get the name of the source UD we want to copy LONG type = dataPos->GetLong(DESC_CUSTOMGUI); //Get the data type setting LONG unit = dataPos->GetLong(DESC_UNIT); //Get the units setting(Real, Percent. etc..) //Set the first UD values to the same as the third DescID first(DescLevel(ID_USERDATA, DTYPE_SUBCONTAINER, 0), 1); ud->Set(first, bcPos, NULL); tag->SetParameter(DescID(first), GeData(dPos), DESCFLAGS_SET_0);Trying your suggestions this is what happened:
1.) AutoPtr<BaseContainer> data( bc->GetClone(COPYFLAGS_0, NULL) );
This compiles and runs for me. But it still leaks memory the same way as before.2.) BaseContainer data;
bc->CopyTo(&data,COPYFLAGS_0,NULL);
I could not get this to compile. const BaseContainer *dataPos = ud->Find(pos); will not let me use this code.
I get this error: can't convert *BaseConatiner to baseContainer.3.)BaseContainer *data = bc->GetClone(COPYFLAGS_0, NULL);
//the rest of my copy code
gDelete(data ); //<--Crashes C4D!!
This one crashes C4D. And I assume that's happening because I'm trying to free it in the same scope as I've created it?
I also tried making the BaseContainer *data variable a class member variable. Then freeing it in the destructor. But C4D still crashes on me.To give you guys a better idea of what I'm battling. Here is the link to my plugin with the source code:https://sites.google.com/site/scottayersmedia/PoseLibrary_R13.zip
I know it's a lot of code to look through. I'm sorry for that.
But I think I have everything working properly except for the UserData section in the GeDialog's Command() method. So you shouldn't need to look through the entire thing.
If I can just get this one last memory problem solved with the UserData. I think the plugin will finally be memory leak free.Thanks for the help,
-ScottA -
On 29/06/2013 at 15:20, xxxxxxxx wrote:
Heap allocated memory does have nothing to do with the scope. Have you confirmed that Cinema
crashes at exactly the gDelete(data); line? It's quite possible that you still try to access the container
although you have already deleted it.Why not use the copy-constructor or assignment operator? They both copy the values from the
original to the new/assigned container. You are already making use of it. To be more precise, in
your code, you are actually doing two copys of the container, which is not necessary.BaseContainer data(bc); BaseContainer data = bc;Do both create legal copies "data" of the container "bc" on the stack (which means you do not
have to take care of the deallocation).-Niklas
-
On 29/06/2013 at 16:42, xxxxxxxx wrote:
I'm not sure If I understand Nik.
The typical problems I'm running into is that certain UserData functions (like BrowseGetNext) require a const container. And others require a pointer based *BaseContainer. And others use a standard non-pointer based BaseContainer.
This is one reason why it's very confusing to use these things together. They all have their little specific restrictions imposed on them. And it gets very confusing.
For example:
The reason I don't just use the bc variable in my custom udSequential() method. Is because Set->() requires a pointer based *BaseContainer().
So the only reason for BaseContainer data = *bc->GetClone(COPYFLAGS_0, NULL); to exist in that method is so I can use it in the: ud->Set(dscID, data, NULL); line of code.When it comes to the code in my Command() method in the GeDialog.
I just noticed that I'm also doing that same thing. But I don't really need to do that, because in that code block I'm only needing to copy the parameter values of a UD (the names, units, field values). And I really don't need to copy or Set->() the ID# stuff.
So I think I can safely remove these from my code:
BaseContainer bcPos = *dataPos->GetClone(COPYFLAGS_0, NULL);
BaseContainer bcRot = *dataRot->GetClone(COPYFLAGS_0, NULL);
Thanks for making me see that.But even with that code deleted. I'm still getting memory leaks from:
const BaseContainer *dataPos = ud->Find(pos);
const BaseContainer *dataRot = ud->Find(rot);And if I try to use gDelete(dataPos); & gDelete(dataRot);
C4D freezes(crashes).-ScottA
-
On 29/06/2013 at 17:39, xxxxxxxx wrote:
Doh!
Doh!
Doh!
I'm such a moron.
-First. Thank you Remo.
This does indeed fix the memory leak problem: AutoPtr<BaseContainer> data( bc->GetClone(COPYFLAGS_0, NULL) );
The reason it didn't work for me before was because I was using it in the wrong place in my code.-Second. Thank you Niklas.
You helped me to see some code I could remove from one of my code blocks.I think that finally squashes all the memory leaks In this plugin.
I'll have to test if for a while to make sure.
Good God this thing was a total nightmare to develop. But I sure did learn a lot from it.Thanks guys,
-ScottA -
On 29/06/2013 at 19:08, xxxxxxxx wrote:
Originally posted by xxxxxxxx
Memory handling has always been necessary in C and C++. And the SDK is very precise
when it comes to information on when memory is yours or the applications memory.That is because many of us long-time developers pushed them to make these clarities. Earlier versions were very difficult to work with because of this lack of ownership information. They have definitely improved upon that extremely well!
-
On 29/06/2013 at 21:20, xxxxxxxx wrote:
Originally posted by xxxxxxxx
On a side note: Is there a functionality in Cinema _somewhere_ that allows userdata to get sorted?
i am not sure what you do mean with sorted in that context, but ther is always bc.sort()
-
On 30/06/2013 at 00:07, xxxxxxxx wrote:
Originally posted by xxxxxxxx
[...]
BaseContainer bcPos = *dataPos->GetClone(COPYFLAGS_0, NULL);
BaseContainer bcRot = *dataRot->GetClone(COPYFLAGS_0, NULL);
Thanks for making me see that.Glad I could help you. Btw, these should result in memory leaks, too!
And sorry, my examples above were not completely correct. Assuming "bc" is a pointer to a
container, you have to dereference it of course!> BaseContainer data = *bc;
>
> BaseContainer data(*bc);Originally posted by xxxxxxxx
That is because many of us long-time developers pushed them to make these clarities. Earlier versions were very difficult to work with because of this lack of ownership information. They have definitely improved upon that extremely well!
I see. You know, I'm not using the C++ SDK for that long I guess..
Originally posted by xxxxxxxx
Originally posted by xxxxxxxx
On a side note: Is there a functionality in Cinema _somewhere_ that allows userdata to get sorted?
i am not sure what you do mean with sorted in that context, but ther is always bc.sort()
I didn't mean sorting a container, but the collection of user-data on an object (which is what
Scott is trying to do). Everything in the SDK is at least used once by Maxon in development. There
is _no_ part in Cinema that results in user-data being re-arranged and sorted. Why should they
include such a functionality in the API? Besides the fact there are quite numerous ways to sort the
data (id, name, value, type, grouped or not, etc) which they can't just handle all because they were
still missing something. A third-party can write its own when necessary and exactly define the data
being sorted the way they need.-Niklas
-
On 30/06/2013 at 09:38, xxxxxxxx wrote:
Originally posted by xxxxxxxx
Why should they include such a functionality in the API?
Because UserData is one of the most used areas of cinema4D. That's why.
When you have an area of the program that virtually everyone uses. Shouldn't that area be one of the most developed sections of the SDK?At bare minimum. I think there should be:
-Built-in functions to set and change ID#s
-Built in sorting for the UD entries(that also removes gaps in ID#s)It's seems very strange to me that they are not in there. And that I had to write such tasks by hand.
-ScottA