CCMenu not safe when MenuItem deleted. + Suggestion about using CCTexture2D instead of texture filename as init for most nodes

CCMenu is not safe when the selecting menu item is deleted (touched, then deleted MenuItem when finger still touching). In function CCMenu::ccTouchEnded and ccTouchCancelled, it uses m_pSelectedItem inited from ccTouchBegan. The menu item node can be deleted without notifying CCMenu. Therefore to use m_pSelectedItem directly is not safe. I don’t know why cocos2dx keeps that pointer, the current selected item can be get through itemForTouch function.

Another suggestion is, except for using texture filename for functions like initWithFile, it is more useful to provide static functions like initWithTexture. Especially for those base nodes such as CCAtlasNode.
I think it is better to use initWithTexture since we can load and free the texture manually.

I added these functions:

static CCAtlasNode * atlasWithTileTexture(CCTexture2D* texture,int tileWidth, int tileHeight, int itemsToRender);
bool initWithTileTexture(CCTexture2D* texture, int tileWidth, int tileHeight, int itemsToRender);
CCAtlasNode * CCAtlasNode::atlasWithTileTexture(CCTexture2D *texture, int tileWidth, int tileHeight, int itemsToRender)
{
    CCAtlasNode * pRet = new CCAtlasNode();
    if (pRet->initWithTileTexture(texture, tileWidth, tileHeight, itemsToRender))
    {
        pRet->autorelease();
        return pRet;
    }
    CC_SAFE_DELETE(pRet);
    return NULL;
}
bool CCAtlasNode::initWithTileTexture(CCTexture2D *texture, int tileWidth, int tileHeight, int itemsToRender)
{
    assert(texture != NULL);
    m_nItemWidth  = (int) (tileWidth * CC_CONTENT_SCALE_FACTOR());
    m_nItemHeight = (int) (tileHeight * CC_CONTENT_SCALE_FACTOR());

    m_cOpacity = 255;
    m_tColor = m_tColorUnmodified = ccWHITE;
    m_bIsOpacityModifyRGB = true;

    m_tBlendFunc.src = CC_BLEND_SRC;
    m_tBlendFunc.dst = CC_BLEND_DST;

    // double retain to avoid the autorelease pool
    // also, using: self.textureAtlas supports re-initialization without leaking
    this->m_pTextureAtlas = new CCTextureAtlas();
    m_pTextureAtlas->initWithTexture(texture, itemsToRender);

    if (! m_pTextureAtlas)
    {
        CCLOG("cocos2d: Could not initialize CCAtlasNode. Invalid Texture.");
        delete this;
        return false;
    }

    this->updateBlendFunc();
    this->updateOpacityModifyRGB();

    this->calculateMaxItems();

    return true;
}

Well I know it is not good since initWithTileTexture is almost the copy of initWithTileFile.

CCMenu::m_pSelectedItem

The source in CCMenu is translated from objc. In objc, calling nil~~>menthod is safe, while in cpp it cause crash. That’s why the source appears as this.
And you mustn’t delete a CCObject in the game source, it’s protected by the reference count mechanism. Use pMenuItem~~>release() instead.

At first, if you want to call pMenuItem~~>release, you need to “own” this CCObject before. pMenuItem~~>retain() is required in your previous source. The codes breaking the rule is not promised to run correctly :slight_smile: And the retain/release/autorelease rule are described here http://www.cocos2d-x.org/boards/6/topics/678?r=696

And then, when the CCMenu::addChild(pMenuItem, …) executes, pMenuItem will be retained by CCMenu, and its ref = 2. So your release once in ccTouchesMoved/Ended/Cancelled will not delete it essentially, its ref = 1 now. MenuItems will live until its owner CCMenu is deleted.

initWithTexture

Cocos2d-x just keep the same methods to cocos2d-iphone, which will make porting games from ios to android much easier. The methods may be a little inconvenient or irrational, but cocos2d-iphone community and RicardoQuesada have their reasons to design as this. We have no plan to make difference interfaces if cocos2d-iphone already designed them. The new features such as muti-platform support, multi-resolution support, and script support modules are exceptions, we can re-design them freely.

Hello,

h5nc Thor wrote:

CCMenu is not safe when the selecting menu item is deleted (touched, then deleted MenuItem when finger still touching). In function CCMenu::ccTouchEnded and ccTouchCancelled, it uses m_pSelectedItem inited from ccTouchBegan. The menu item node can be deleted without notifying CCMenu. Therefore to use m_pSelectedItem directly is not safe. I don’t know why cocos2dx keeps that pointer, the current selected item can be get through itemForTouch function.

Thereby crash occurs in CCMenu::onExit() method when m_pSelectedItem pointer is NULL.

You can easily reproduce it:

  • tap menu item (CCMenu::ccTouchBegan: m_pSelectedItem != NULL; m_eState = kCCMenuStateTrackingTouch)
  • move finger away (CCMenu::ccTouchMoved: m_pSelectedItem = NULL)
  • and remove CCMenu node from the parent.

Then CCMenu::onExit called before CCMenu::ccTouchEnded or CCMenu::ccTouchCancelled, so m_eState is still equal kCCMenuStateTrackingTouch and the application crashes on m_pSelectedItem->unselected().

Tested in cocos2d-2.0-x-2.0.2, but seems bug also wasn’t fixed in the latest version.