On 10/15/2018 at 8:55 PM, WitchLord said:

This problem you've faced is common too all languages with automatic garbage collection, e.g. Java, C#, etc. We who are used to C++ ways of thinking just have to learn to think differently than we are used to in these cases There are pro's and con's with both automatic GC and manual GC.

Maybe you could at avoid this extra effort for the script writers by having it done automatically by your engine. Instead of the script providing the cleanup function, the engine could just enumerate all members of the script object and set all handles to null (or perhaps only the ones that are critical C++ objects).

Hmm that sounds like a good idea! I'd have some way to identify these. One could be just having a list of the classes that are registered like this (there is limited set of those) and finding all occurences of such class in a object, then releasing it. Or more generic approach with some metainfo like [autoclean] - but first solution may be better considering there won't be a lot of these classes. Thanks, for some reason I haven't thought to use built-in script reflection for this kind of cleanup!

I managed to solve most of my problems, but encountered a new one - and since it's related to refcounting script registered OBJ_REF classes, I thought I will continue here rather than start a new topic. I've spent some hours debugging this and I have literally no idea why this happens, so hope @WitchLord or anyone knowing AS internals much better than me will be able to help. 

asCHECK(engine->RegisterObjectType("CollisionShape", 0, asOBJ_REF));
asCHECK(engine->RegisterObjectBehaviour("CollisionShape", asBEHAVE_ADDREF, "void f()",  asMETHOD(CollisionShapeScriptProxy, addRef), asCALL_THISCALL));
asCHECK(engine->RegisterObjectBehaviour("CollisionShape", asBEHAVE_RELEASE, "void f()", asMETHOD(CollisionShapeScriptProxy, release), asCALL_THISCALL));
asCHECK(engine->SetDefaultNamespace("CollisionShape"));
asCHECK(engine->RegisterGlobalFunction("CollisionShape@ makeSphere(float)", asMETHOD(CollisionShapeFactory, makeSphere), asCALL_THISCALL_ASGLOBAL, this));
asCHECK(engine->RegisterGlobalFunction("CollisionShape@ makePlane(const vec3 &in, float)", asMETHOD(CollisionShapeFactory, makePlane), asCALL_THISCALL_ASGLOBAL, this));
asCHECK(engine->RegisterGlobalFunction("CollisionShape@ makeBox(const vec3 &in)", asMETHOD(CollisionShapeFactory, makeBox), asCALL_THISCALL_ASGLOBAL, this));
asCHECK(engine->SetDefaultNamespace(""));

Important note on what's different here than my other classes (which work fine!): this class has no factory registered, because I want to have several factory functions which are registered as globals to make different kinds of the same CollisionShape.

Now, the class has the same mechanism of refcounting as my other component-classes (transform, animator, skeleton etc.) but only this class has weird behaviour of AddRef happening one time too many. Lifetime of this class is as follow:

1. Inside Script Object it's created via  

@collisionShape = CollisionShape::makeBox(...);

2. Default ref count is 1, class is created with new CollisionShapeProxy, then it's destroyed when refcount reaches 0. 

3. CollisionShape is passed to RigidBody which AddRefs it during its lifetime then Releases, but the problem does not lie there, and to rule it out I stopped affecting refcount when it's passed to RigidBody for now. So all that keeps hold on that object is @collisionShape assignment shown above.

4. When I destroy the object, it cleans up doing this:

@collisionShape = null;

So this should effectively delete the proxy because neither RigidBody nor ScriptObject holds reference anymore... but it doesn't. All other objects behave like this but this one stays on 1 refcount for some unknown reason. I started dumping all addrefs & releases including few lines of callstack and this is what happens during its lifetime:

1  2018-10-21 21:08:59,136891 slot_map.h:88 [VERBOSE] [class CollisionShape] Creating new element @ sparseIndex 0, version: 0
2  2018-10-21 21:09:02,875345 slotmap_script_proxy.h:61 [VERBOSE] class CollisionShape[1370FB38]++ (1) -> 
3   asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4069]
4   asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4013]
5   asCContext::ExecuteNext [as_context.cpp:2893]
6   asCContext::Execute [as_context.cpp:1328]
7  2018-10-21 21:09:02,875345 slotmap_script_proxy.h:67 [VERBOSE] class CollisionShape[1370FB38]-- (2) -> 
8   asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4069]
9   asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4013]
10  asCContext::ExecuteNext [as_context.cpp:2815]
11  asCContext::Execute [as_context.cpp:1328]
12 2018-10-21 21:09:06,515185 slotmap_script_proxy.h:61 [VERBOSE] class CollisionShape[1370FB38]++ (1) -> 
13 asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4069]
14 asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4013]
15 asCContext::ExecuteNext [as_context.cpp:4152]
16 asCContext::Execute [as_context.cpp:1328]
--- here script object is destroyed, all resources released (except CollisionShape which stays alive ---
17 2018-10-21 21:09:08,394326 slotmap_script_proxy.h:67 [VERBOSE] class CollisionShape[1370FB38]-- (2) -> 
18 asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4069]
19 asCScriptEngine::CallObjectMethod [as_scriptengine.cpp:4013]
20 asCContext::ExecuteNext [as_context.cpp:2888]
21 asCContext::Execute [as_context.cpp:1328]                                                    

Numbers in ( ) are refCount BEFORE it's decreased or increased. So we begin with line 2, refcount (1) which is initial refcount and it's increased (2).
Then it's decreased on line 7, so we're back to (1), but then again it's increased to (2) on line 12 and it stays like this. So the only thing that's referencing
our CollisionShape is the script, and somehow it holds 2 references. Then at line 17 we have decref due to destruction, so it goes down to (1) and stays
like this forever - it will never be cleaned up. Nothing like this happens with other classes using same base refcounted class... The only difference I see
is that this class is the only one made not through factory behaviour but global function, but it has the same declaration as factory.

I took screenshots of these 3 calls, inside context where it seems to be most relevant - 2 addrefs & 1 release:

\

I have no idea what happens here - seems like there are two things that increase ref: asBC_REFCPY, then asBC_RefCpyV, but only one that decreases asBC_FREE. This leaves the proxy refcount at 2 (initial 1 + non-released ref during script execution). The whole thing happens during construction of script object that holds the reference, and CollisionShape@ is created inside its constructor like this:

So... if anyone has any idea what's going on with this additional addRef - please halp!  Sorry for that long post, but I lack the insight to follow what's going on there regarding lifetime and refcounting - I just don't know when AS increases those and when not etc. I hope that by providing that much information someone will spot the problem..

It seems to me that your makeBox function is returning the reference to the collisionShape with the refCount already set to 2. Besides creating the actual instance is makeBox keeping a reference somewhere to warrant the refCount to be 2 when returning?

UPDATE: SOLVED BY RUBBER-DUCKING

Thanks for looking at it! The problem is, I can't see any obvious trace of it, nor I recall writing any. Searching through the code also does not show anything, especially when compared to other classes behaviour which is fine. Also - wouldn't it not show like this on the callstack where it looks like 2 refadds are initiated from within script engine, not from the outside? There should be refadd & refdel when AS is dealing with the returned object, but where does that second refadd comes from? It's also initiated within scripting. 

This is debugger just before the created instance is handed over to AngelScript:

Here is how makeBox looks:

CollisionShapeScriptProxy* CollisionShapeFactory::makeBox(const glm::vec3& halfExtents)
{
   auto handle = store_.emplace(CollisionShape::Type::Box, std::make_unique<btBoxShape>(btVector3(halfExtents.x, halfExtents.y, halfExtents.z)));
   return makeCollisionShapeScriptProxy(handle);
}

That's makeBox - it creates an instance (store_.emplace) but this has nothing to do with scripting, it's C++ only and not refcounted. Then it passes the created instance to proxy, which does this:

CollisionShapeScriptProxy* CollisionShapeFactory::makeCollisionShapeScriptProxy(CollisionShapes::Handle& handle)
{
   return new CollisionShapeScriptProxy(store_, handle, 1);
}

Last parameter is initial refCount passed - I was trying to work around the problem, knowing there is one refcount too many coming from the scripting, and for this class only I tried to pass 0 (->release() would not work because it would cause object to be prematurely destroyed) but it didn't solve the issue and crashed. So that's all I have on non-Angelscript side, this call above is what is registered as 

asCHECK(engine->RegisterGlobalFunction("CollisionShape@ makeBox(const vec3 &in)", asMETHOD(CollisionShapeFactory, makeBox), asCALL_THISCALL_ASGLOBAL, this));

later. When CollisionShapeScriptProxy is created above, its refcount is 1 and then something happens inside the script that changes it - the code for script itself I already pasted. 

Just for the sake of completion this is part of the base ScriptProxy class that implements refCounting for all the classes (the ones that work and CollisionShape):

ScriptProxy(SlotMap<T>& container, const typename SlotMap<T>::Handle& handle) :
    ScriptProxy(container, handle, 1)
 {
 }
 
 ScriptProxy(SlotMap<T>& container, const typename SlotMap<T>::Handle& handle, int16_t initialRefcount) :
    containerRef_(container),
    handle_(handle),
    refCount_(initialRefcount)
 {
 }

void addRef()
{
   VLOG(3) << typeid(T).name() << "[" << this << "]++ (" << refCount_ << ") -> \n" << getCallStack(2,6);
   refCount_++;
}

void release()
{
   VLOG(3) << typeid(T).name() << "[" << this << "]-- (" << refCount_ << ") -> \n" << getCallStack(2,6);
   CHECK(handle_ != SlotMap<T>::NullHandle);
   if (--refCount_ == 0)
      delete this;
}

SOLVED BY RUBBER-DUCKING

@WitchLord, one quick idea that made me wonder - the only difference is creation through global "factory function", not factory itself - isn't it like: for factory behaviour, AS does not increase refcount on top of what it does for handling of the call, because it assumes user is setting refCount to 1 when returning new object - but global function does not assume this, and it additionally increases refCount of the object returned from it? So we end up with 1 refcount from constructor and 1 refcount from Angelscript, even though we don't want it because method is acting like factory and already returned refcount 1? 

That's why I had idea to initially set it to 0 for creation from method and not factory behaviour, but the order in which it happens is: ++, --, ++, RETURN which meant that if we 0++, then 1-- it ends up deleting itself. I think I can't do much because once I return that object from my C++ makeBox(), it's then processed by AngelScript.

Andreas, can you confirm the above behaviour for registered global function that would increase refcount when returning something declared like this? And that its indeed different than factory behaviour?

asCHECK(engine->RegisterGlobalFunction("CollisionShape@ makeBox(const vec3 &in)", asMETHOD(CollisionShapeFactory, makeBox), asCALL_THISCALL_ASGLOBAL, this));

I guess it totally make sense for non-factory type of functions, especially when Foo@ is a script object (this has to happen because on script side you have no way to increase refcount), but if FooCpp@ is actually on C++ side we can do this manually when returning the object. Most of the time we also want this, but not for factory functions. 

How to work around it when one can have more factory methods and not necessarily just create through constructor. Here I'd actually need "CollisionShape@-" kind of thing it seems. I wanted to make it explicit by the func name what's created: makeBox, makeSphere, makeCapsule because if I did it thorugh ctor it would not be encapsulated and have to be more in way of CollisionShape(Shapes::Box, boxparam1, boxparam2) which is not really good and can cause problems if user mismatches params & type enum. So the possibility to have global factory functions not registered as behaviour would be great.

Although according to the docs this should be possible:

A function that creates an object and returns it to the script engine might look like this:

// Registered as "obj@ CreateObject()"
obj *CreateObject()
{
  // The constructor already initializes the ref count to 1
  return new obj();
}

I also checked, just in case, behaviour of calling this (a bit nonsensical) code:

shared class Plane: Std::TangibleObject
{
    Plane()
    {
        CollisionShape::makePlane(vec3(0, 1, 0), 0.0f);
    }
    
    void cleanUp() override
    {
        TangibleObject::cleanUp();
        @rigidBody = null;
        @collisionShape = null;
        @mesh = null;
    }

    CollisionShape@ collisionShape;
    RigidBody@ rigidBody;
    StaticMesh@ mesh;
}

and it seems to result in just single FREE, taking refCount from 1(initial, from ctor) -> 0 and removing the created object asap. This works without crashes - just not sure why it FREEs without adding ref, so it must expect the outside the be adding ref probably? It's not that clear to me how AS calls addRef/release during various situations - I will try to make some diagram once I get the hang of it, but the case with factory function is a weird one. 

I tried to make a simplified test case and it's getting weird... In the test case it does not do this additional addRef which causes the trouble:

So it seems that it DOES work as expected even when using factory global function, so something must be wrong in my code that causes this addRef

*SOLVED*

This forum becomes sometimes my rubber duck.. I don't know how I missed that... I forgot the fact that when passing reference to a method, it's addRef'd, but then release is up on the user. So it was addRefing when passing CollisionObject to RigidBody constructor... I had this in my RigidBody. And when I removed it to "not affect" the CollisionShape - I removed both - unnecessary AddRef but also Release, the one that will zero the ref count in the very end...
 

class Rigidbody
{
   Rigidbody(CollisionObjectProxy* collisionObject, ...) {
      collisionObject_ = collisionObject;
      collisionObject_->addRef();    // <<<<<<-------- THIS IS NOT NEEDED BECAUSE ANGELSCRIPT ALREADY ADDREF'D COLLISION OBJECT
                                     //                WHEN IT WAS PASSED TO RigidBody() script constructor
   }

   ~RigidBody() { collisionObject->release(); }
}

and was doing this:
asCHECK(engine->RegisterObjectBehaviour("RigidBody", asBEHAVE_FACTORY, 
                                        "RigidBody@ f(Transform@, CollisionShape@, float)", asMETHOD(DynamicsWorld, makeRigidBody), 
                                                                  // ^^^^^^^^^^^^^^^ addRef, but release up to user

which created RigidBody(...)

It's actually in documentation (https://www.angelcode.com/angelscript/sdk/docs/manual/doc_obj_handle.html) but it wasn't about constructing CollisionShape, it was about passing it to other constructor. I will try to create some refadd/release flow for AS, maybe it helps others too, as it's sometimes easy to miss when script does something for you and when it wants you to do something...

I'm glad you figured out the cause of the problem.

There is absolutely no problem in using the forum as a rubber duck. I'd wager that a large percentage of the members here have also done the same. I too often have the same experience. Just being forced to describe the problem makes you think in different ways which often lets you see the cause of the problem you couldn't otherwise see.