axmolengine / axmol

Axmol Engine – A Multi-platform Engine for Desktop, XBOX (UWP) and Mobile games. (A fork of Cocos2d-x-4.0)
https://axmol.dev
MIT License
923 stars 205 forks source link

RenderTexture::getContentSize() is never set and always returns Size(0, 0) #1101

Closed rh101 closed 1 year ago

rh101 commented 1 year ago

Calling RenderTexture::getContentSize() always results in (0, 0) (same in Cocos2d-x). Is there any reason why this is so?

This also impacts RenderTexture::getBoundingBox(), which has fields that are all 0.

It seems to make more sense to set the content size based on the size the RenderTexture is created with.

This would mean that we would no longer need to call: RenderTexture->getsprite()->getConentSize() RenderTexture->getsprite()->getBoundingBox()

and can simply call RenderTexture->getContentSize() RenderTexture->getBoudingBox()

If there is no good reason why it is returning (0, 0), then I suggest this be changed to either set the content size of RenderTexture on creation, or have RenderTexture::getContentSize() return the internal sprite getContentSize() (which would need to be done for getBoundingBox() too).

Any input regarding this would be appreciated.

rh101 commented 1 year ago

If this is fixed, then the internal _sprite position would also need to be changed. With the current code, the sprite anchor point is at Vec2::ANCHOR_MIDDLE, and given the size of the RenderTexture is Vec2(0, 0), then it just happens that the sprite looks like it is correctly positioned.

If the content size of the RenderTexture is set to a value other than Vec2(0, 0), then this will affect the internal sprite position, so it would need to also be adjusted to be centered inside the RenderTexture size, with either: _sprite->setPositionNormalized(Vec(0.5f, 0.5f)); or _sprite->setPosition(getContentSize() / 2);

I've discovered other issues with RenderTexture as well, such as the RenderTexture::setAutoDraw(true); not working as intended. The implementation of the draw method seems to be incorrect:

void RenderTexture::draw(Renderer* renderer, const Mat4& transform, uint32_t flags)
{
    if (_autoDraw)
    {
        // Begin will create a render group using new render target
        begin();

        // clear screen
        _director->getRenderer()->clear(_clearFlags, _clearColor, _clearDepth, _clearStencil, _globalZOrder);

        //! make sure all children are drawn
        sortAllChildren();

        for (const auto& child : _children)
        {
            if (child != _sprite)
                child->visit(renderer, transform, flags);
        }

        // End will pop the current render group
        end();
    }
}

This will simple draw a blank image. What I've found to work correctly is to remove the begin()/end() and also remove the call to _director->getRenderer()->clear(...):

void RenderTexture::draw(Renderer* renderer, const Mat4& transform, uint32_t flags)
{
    if (_autoDraw)
    {
        //! make sure all children are drawn
        sortAllChildren();

        for (const auto& child : _children)
        {
            if (child == _sprite)
                continue;

            child->visit(renderer, transform, flags);
        }
    }
}
rh101 commented 1 year ago

The alternative to fixing getContentSize(), getBoundingBox() and such is to add all the methods that don't work correctly under the private access modifier, so they cannot be called by external code, forcing the usage of RenderTexture::getSprite()->getContentSize() etc.

rh101 commented 1 year ago

Is there any point in fixing/changing/improving the RenderTexture class, or should it just be left as-is?

rh101 commented 1 year ago

I'll look into addressing the issues mentioned above and enhancing the API for RenderTexture, such as adding a RenderTexture::getTexture() method, which would make a lot of sense for that class. Since no-one else seems to be having issues with the RenderTexture functionality in its current state, then changing it isn't a priority.