Retain _debugDrawNode in Label

[TyelorD]

Describe your changes

This fixes the memory corruption bug and resulting crash I outlined in the TextFieldTTF (and likely other TextField classes) where the _debugDrawNode wasn't being retained, and the label isn't added to the scene for a TextField, but is maintained by the TextField. This caused _debugDrawNode in the Label class to get cleaned up, even though Label still thought it had a valid reference to a DrawNode.

Issue ticket number and link

1889

Checklist before requesting a review

For each PR

• [x] Add Copyright if it missed:
  • "Copyright (c) 2019-present Axmol Engine contributors (see AUTHORS.md)."

• [x] I have performed a self-review of my code.

  Optional:

  • [ ] I have checked readme and add important infos to this PR.
  • [ ] I have added/adapted some tests too.

For core/new feature PR

• [ ] I have checked readme and add important infos to this PR.
• [ ] I have added thorough tests.

[TyelorD]

I should note, that while this does fix the crash I was seeing, it also causes the TextField to lose its debug bounding box still. I'm not sure why that would be the case however.

[rh101]

This caused _debugDrawNode in the Label class to get cleaned up, even though Label still thought it had a valid reference to a DrawNode.

That is strange. The DrawNode is created then added as a child:

#if AX_LABEL_DEBUG_DRAW
    _debugDrawNode = DrawNode::create();
    addChild(_debugDrawNode);
#endif

At the end of that line, the _debugDrawNode reference count = 2, and by the end of the next update, it's been removed from the auto-release pool. so the reference count drops to 1 (since it's still a child of Label). It should not matter if the label is added to the scene or not, since the label is also held as a child of TextFieldEx, so it would also be valid:

this->renderLabel =
    createLabel(placeholder, fontName, fontSize, Vec2::ZERO, TextHAlignment::CENTER, TextVAlignment::CENTER);
this->renderLabel->setAnchorPoint(Point::ANCHOR_MIDDLE_LEFT);
this->addChild(this->renderLabel);

The only reason _debugDrawNode would be invalid is if it was removed as a child of Label and _debugDrawNode is never set to nullptr; this would happen with calls to removeAllChildren etc..

Explicitly retaining and releasing is the right thing to do when keeping references to such objects, but in this case, it may also be covering up the root cause of the issue that you're facing.

This caused _debugDrawNode in the Label class to get cleaned up, even though Label still thought it had a valid reference to a DrawNode.

Do you mean it has been removed as a child of Label? Perhaps finding out why this is the case will lead you to the source of the problem, because retaining and releasing is not going to fix the issue of the DrawNode being removed as a child of the label. It would just be a holding a reference to an object that is no longer in the scene, which may explain why you're not seeing anything being drawn on the screen by the DrawNode.

[rh101]

The call to UICCTextField::openIME() eventually ends up in a call to Label::removeAllChildrenWithCleanup(), as you can see here:

Putting a memory breakpoint on the _referenceCount field of the _debugDrawNode that breaks when the value changes quickly showed the problem.

In addition to your change regarding retaining/releasing, you would also need to add code in Label::removeAllChildrenWithCleanup to re-add the _debugDrawNode to the Label via a call to addChild(_debugDrawNode):

void Label::removeAllChildrenWithCleanup(bool cleanup)
{
    Node::removeAllChildrenWithCleanup(cleanup);
    _letters.clear();
#if AX_LABEL_DEBUG_DRAW
    addChild(_debugDrawNode);
#endif
}

Also it would need to be added to Label::reset(), since that bypasses Label::removeAllChildrenWithCleanup(), and instead calls Node::removeAllChildrenWithCleanup(true);:

void Label::reset()
{
...
...
#if AX_LABEL_DEBUG_DRAW
    if (_debugDrawNode)
    {
        addChild(_debugDrawNode);
    }
#endif
}

In order for the bit of code in Label::reset() not to crash due to the call order, _debugDrawNode needs to be initialised to nullptr before the constructor code is executed. Easiest is to just set _debugDrawNode = nullptr; in the header file.

It is also a little odd why the _debugDrawNode is created in the constructor and not in one of the init() methods.

With all the changes combined, the text box outline will no longer disappear.

[smilediver]

Maybe Label shouldn't add DrawNode to the children at all and call DrawNode::visit() in Label::visit() directly?

But I think it's only a symptom... the actual bug is that TextFieldTTF::setString calls removeAllChildrenWithCleanup(). It shouldn't, because it can screw up user's attached nodes.

[halx99]

Try use UITextFieldEx instead, we should deprecated 2d/TextFieldTTF and ui/UITextField

TextFieldEx has better cursor support

[rh101]

@TyelorD Alternative solution to what I mentioned previously is precisely what @smilediver pointed out, which is to call visit(), since the drawn box should always be visible.

For this, ignore the changes in my previous post, and also remove any addChild(_debugDrawNode); call.

So in the end, the only modifications would just be the addition of the AX_SAFE_RETAIN/AX_SAFE_RELEASE, removal of the addChild(_debugDrawNode), and the addition of the code in visit(), which would be towards the end of the method, like this:

void Label::visit(Renderer* renderer, const Mat4& parentTransform, uint32_t parentFlags)
{
...
...
...
#if AX_LABEL_DEBUG_DRAW
    _debugDrawNode->visit(renderer, _modelViewTransform, parentFlags | FLAGS_TRANSFORM_DIRTY);
#endif

    _director->popMatrix(MATRIX_STACK_TYPE::MATRIX_STACK_MODELVIEW);
}

[TyelorD]

@TyelorD Alternative solution to what I mentioned previously is precisely what @smilediver pointed out, which is to call visit(), since the drawn box should always be visible.

Alright, I think that sounds like the "more proper" solution of the two proposed fixes for my change, so I changed Label.cpp to only retain/release the _debugDrawNode instead of also adding it as a child, and then added the lines above to Label::visit.

One thing to note that we may want to amend in the future (or at least discuss how to handle) is that the insertion cursor renders outside the labels bounding box, and so it's not captured by the _debugDrawNode. Maybe this is what we want, but maybe it isn't either. But this would require much larger changes to the TextField classes, and personally I'm not sure it'd be worth it anyways. It probably wouldn't be hard to just manually add the width of the cursor if it were needed.

[rh101]

One thing to note that we may want to amend in the future (or at least discuss how to handle) is that the insertion cursor renders outside the labels bounding box, and so it's not captured by the _debugDrawNode. Maybe this is what we want, but maybe it isn't either. But this would require much larger changes to the TextField classes, and personally I'm not sure it'd be worth it anyways. It probably wouldn't be hard to just manually add the width of the cursor if it were needed.

AX_LABEL_DEBUG_DRAW is purely for the boundary of the label, so it is working correctly. The insertion cursor is not part of the Label, but rather a part of the TextFieldTTF. If you need to see the boundary of the TextFieldTTF, then you have to use a separate draw node for that, which would actually encapsulate everything.

[rh101]

@TyelorD Is there anything else to be added to this PR?

[rh101]

@halx99 The last commit made should fix the issue, and unless @TyelorD has any objections, this PR seems to be ready to merge.