Iterating through cc.Node children breaks on v2.1.4

jpoloney · June 19, 2013, 7:13pm

We updated from cocos2dx 2.1.3 to 2.1.4 and are running into some problems iterating over CCNode children. The problem is with this commit: https://github.com/cocos2d/cocos2d-x/commit/2f4a0ed37c6686a521a32c8301d48751288d3292.

On line 90, you add the JSB_ScrollViewDelegate as a child in js_cocos2dx_CCScrollView_setDelegate(). If you iterate through all of the child nodes in JS, it hits the assert in ccarray_to_jsval(). I’m not sure why the JSB_ScrollViewDelegate is failing the proxy check at the beginning:

js_proxy_t* jsproxy = js_get_or_create_proxy(cx, obj);
if (jsproxy) {
     arrElement = OBJECT_TO_JSVAL(jsproxy->obj);
}

Am I forgetting to include some files in my project? Or is this a potential bug?

Thanks,

— Joel

dumganhar · June 20, 2013, 3:38am

Hey, @Joel Poloney,
How did you iterate through all of children nodes in JS?

mutoo · June 20, 2013, 4:19am

`James It seems that getChilden() automatically refer by engine:

JSBool js_cocos2dx_CCNode_getChildren(JSContext *cx, uint32_t argc, jsval vp)
{
…
if (argc == 0) {
cocos2d::CCArray ret = cobj->getChildren();
jsval jsret;
jsret = ccarray_to_jsval(cx, ret);
JS_SET_RVAL(cx, vp, jsret);
return JS_TRUE;
}
…
}

This probleam cause by a JSB_ScrollViewDelegate being added to a CCScrollView.m_pContainer, but

js_proxy_t* jsproxy = js_get_or_create_proxycocos2d::CCObject(cx, obj);

couldnot recognize the JSB_ScrollViewDelegate in

jsval ccarray_to_jsval(JSContext* cx, CCArray *arr)

James Chen wrote:

Hey, `Joel Poloney,
How did you iterate through all of children nodes in JS?

jpoloney · June 20, 2013, 4:25am

Yep, Mutoo Lau is correct. That’s where we’re seeing the error too.

— Joel

dumganhar · June 20, 2013, 4:49am

I didn’t find a good way to delete instance that created by new JSB_ScrollViewDelegate.
Since CCScrollView::setDelegate is a weak reference function, I don’t know when the JSB_ScrollViewDelegate needs to be deleted.
Therefore, I added it to the CCScrollView and make JSB_ScrollViewDelegate inherited from CCNode.
Then, JSB_ScrollViewDelegate will get notification when its parent is onExit.
I didn’t find a better way to resolve this issue.

Any suggestion or solution will be appreciated. Thanks.

jpoloney · June 20, 2013, 4:56am

@James Chen - JSB_ScrollViewDelegate is a CCNode, right? Shouldn’t the proxy be resolving the object correctly?

js_proxy_t* jsproxy = js_get_or_create_proxy(cx, obj);
if (jsproxy) {
     arrElement = OBJECT_TO_JSVAL(jsproxy->obj);
}

That’s what I think was intended, but it doesn’t appear to actually be working. Thoughts?

dumganhar · June 20, 2013, 5:39am

@joel Poloney,
I found a solution to resolve this issue:

Make JSB_ScrollViewDelegate to inherit from CCObject rather than CCNode.
Use cobj->setUserObject* rather thanaddChild*.

Could you please try and give me the feedback?
Please make sure that the destructor of JSB_ScrollViewDelegate is invoked. Sorry for this issue.

jpoloney · June 20, 2013, 5:51am

`James Chen

What do you mean by invoking the destructor of JSB_ScrollViewDelegate? If I make these two changes, I’m getting compile errors on `oldDelegate->removeFromParent();@

If I comment that line out, it appears to work fine.

dumganhar · June 20, 2013, 6:11am

oldDelegate->removeFromParent();

Use cobj->setUserObject; instead.

What do you mean by invoking the destructor of JSB_ScrollViewDelegate?

I means that you should make a break point in the destructor of JSB_ScrollViewDelegate to ensure that it was invoked.

jpoloney · June 20, 2013, 6:16am

Yep, that appears to work. Should I make the same changes to CCTableView and CCEditBox as well?

mutoo · June 20, 2013, 6:20am

I think so, the same problems occur to CCTableView (JSB_TableViewDataSource also);

Joel Poloney wrote:

Yep, that appears to work. Should I make the same changes to CCTableView and CCEditBox as well?

dumganhar · June 20, 2013, 6:22am

Yes, the binding of CCTableView, CCEditBox also need to be updated.
@Joel Poloney
Was the destructor of JSB_ScrollViewDelegate invoked?

Mutoo Lau wrote:

I think so, the same problems occur to CCTableView (JSB_TableViewDataSource also);
>
Joel Poloney wrote:
> Yep, that appears to work. Should I make the same changes to CCTableView and CCEditBox as well?

jpoloney · June 20, 2013, 6:27am

James Chen wrote:

@Joel Poloney
Was the destructor of JSB_ScrollViewDelegate invoked?
>

We only ever set the delegate once, so there’s no old delegate to remove. So, I added the same delegate again in our JS (called setDelegate twice in a row) and it went through to the destructor just fine.

dumganhar · June 20, 2013, 6:29am

Yes, cobj->setUserObject(NULL); is not needed indeed. Glad to hear that it works now.

Joel Poloney wrote:

James Chen wrote:
> @Joel Poloney
> Was the destructor of JSB_ScrollViewDelegate invoked?
>
>
We only ever set the delegate once, so there’s no old delegate to remove. So, I added the same delegate again in our JS (called setDelegate twice in a row) and it went through to the destructor just fine.

mutoo · June 20, 2013, 9:19am

I tried this solution for JSB_TableViewDataSource but failed;

I think it’s because of a CCTableView may need a JSB_TableViewDelegate and a JSB_TableViewDataSource but cobj~~>setUserObject just keeps one;
James Chen wrote:

Yes, cobj~~>setUserObject(NULL); is not needed indeed. Glad to hear that it works now.
>
Joel Poloney wrote:
> James Chen wrote:
> > @Joel Poloney
> > Was the destructor of JSB_ScrollViewDelegate invoked?
> >
>
> We only ever set the delegate once, so there’s no old delegate to remove. So, I added the same delegate again in our JS (called setDelegate twice in a row) and it went through to the destructor just fine.

dumganhar · June 20, 2013, 9:21am

Yes, it’s a problem…… No thoughts about that now….

Mutoo Lau wrote:

I tried this solution for JSB_TableViewDataSource but failed;
>
I think it’s because of a CCTableView may need a JSB_TableViewDelegate and a JSB_TableViewDataSource but cobj~~>setUserObject just keeps one;
>
James Chen wrote:
> Yes, cobj~~>setUserObject(NULL); is not needed indeed. Glad to hear that it works now.
>
> Joel Poloney wrote:
> > James Chen wrote:
> > > @Joel Poloney
> > > Was the destructor of JSB_ScrollViewDelegate invoked?
> > >
> >
> > We only ever set the delegate once, so there’s no old delegate to remove. So, I added the same delegate again in our JS (called setDelegate twice in a row) and it went through to the destructor just fine.

dumganhar · June 20, 2013, 9:25am

Hey, I think you could create a CCArray, and set the CCArray to setUserObject. That’ll be able to work.

mutoo · June 20, 2013, 10:04am

It would not be a good idea…

JSB_TableViewDelegate and JSB_TableViewDataSource aren’t in the same place to be added into CCTableView;

James Chen wrote:

Hey, I think you could create a CCArray, and set the CCArray to setUserObject. That’ll be able to work.

dumganhar · June 20, 2013, 12:55pm

No good idea now.

Mutoo Lau wrote:

It would not be a good idea…
>
JSB_TableViewDelegate and JSB_TableViewDataSource aren’t in the same place to be added into CCTableView;
>
James Chen wrote:
> Hey, I think you could create a CCArray, and set the CCArray to setUserObject. That’ll be able to work.

dumganhar · June 20, 2013, 2:51pm

Issue #2315 was created.