Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the Anchor Point issue and prepend the methods calls with this->. #16466

Closed
wants to merge 1 commit into from

Conversation

@N2OMatt
Copy link
Contributor

N2OMatt commented Aug 30, 2016

We are setting the anchor point to the center of the Menu
so the call of setIgnoreAnchorPointForPosition must be with
"false" argument.

Also prepend all the methods calls with this to keep the
consistency between the calls.

We are setting the anchor point to the center of the Menu
so the call of setIgnoreAnchorPointForPosition must be with
"true" argument.
        //
Also prepend all the methods calls with this to keep the
consistency between the calls.
@minggo

This comment has been minimized.

Copy link
Contributor

minggo commented Aug 31, 2016

I am not sure if it is designed like this. @ricardoquesada could you please take a look? Thanks.

@ricardoquesada ricardoquesada self-assigned this Aug 31, 2016
@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Aug 31, 2016

@minggo I think that with "my" fix the correct is achieved.
Why? If we just create an menu like

// In a MyScene::init for example
auto pMenu = cc::Menu::create();
this->addChild(pMenu);

The menu will be placed like that:

       ***********
+------*----+    *
|      *     |   *
|      ***** |****
+-----------+

So to remedy this we must do:

// In a MyScene::init for example
auto pMenu = cc::Menu::create();
pMenu->setIgnoreAnchorPointForPosition(false);
this->addChild(pMenu);

Now the Menu is placed exactly on on the scene.

I think that the original behaviour is very odd,
and entered in the CCMenu.cpp to check why is that
way. For my surprise the call that is required to
cocos2d-x users to do to place the Menu correctly
already is inside of the cocos2dx, but it is "wrong"
(in my view).

Another point that reinforces this (or I really don't
know the internals of cocos2d-x yet) is that:

  • If you set the ignoreAnchorPointForPosition to true
    it will do not take in account the anchor point,
    so the anchor point could be any value (0.5, 0.5) (1.0, 0.5) etc

    But the method is explicitly setting the anchor
    point to the center of Menu, so is "clear" that
    the code writer wanted that the anchor point to be
    the center, and possible mistake the setIgnoreAnchorPointForPosition call.

@ricardoquesada

This comment has been minimized.

Copy link
Contributor

ricardoquesada commented Aug 31, 2016

@N2OMatt you might be right that the default should be with false, but I'm afraid that if we apply this PR it will break backwards compatibility. please, correct me if I'm wrong.

@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Aug 31, 2016

@ricardoquesada I'm doing a couple of tests here... I'll get back in a few minutes.

@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Aug 31, 2016

So @ricardoquesada @minggo I really think that this PR should not break any
backward compatibility. Because the old behaviour is wrong and users should felt it.

First I change the base class of coco2d::Menu from cocos2d::Layer to cocos2d::LayerColor.
Second I added a cocos2d::LayerColor (the red one) into a Scene and a cocos2d::Menu
into the same scene (The code bellow). We can see that the default the cocos2d::Menu is
misplaced.

I doubt that any one is using this straight, without setting the Menu's properties before.

The code that I used is:

 auto pLayer = cc::LayerColor::create(cc::Color4B::RED, 
                                         this->getContentSize().width, 
                                         this->getContentSize().height);
    this->addChild(pLayer);

    auto pMenu = cc::Menu::create();
    this->addChild(pMenu);

And the result that i had was:
screenshot at 2016-08-31 01 33 01

The cocos2d::Layer is the red one and cocos2d::Menu is the blue one.

PS: We can have some trouble if the guy that is using the cocos2d::Menu is changing it's position to offset the wrong Menu placement, but again I think that is very unliked since the change the setIgnoreAnchorPointForPosition to false is more obvious.

What do you think?

@ricardoquesada

This comment has been minimized.

Copy link
Contributor

ricardoquesada commented Aug 31, 2016

@N2OMatt let me do some tests.

btw, it is great to see cocos2d-x linux users!

@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Aug 31, 2016

@ricardoquesada Ok, let me know anything that you discover :D
Glad to be using coco2d-x in Linux (I hope that I can help bring linux to a status of 1st class game system).

@stevetranby

This comment has been minimized.

Copy link
Contributor

stevetranby commented Sep 2, 2016

@N2OMatt this will break existing code. Notice that it's actually redundant for Menu to set ignore and anchor as it's already set the same in Layer.

For now you can just create a utility function to an inline static method CreateMenu() that also sets ignore to false.

@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Sep 2, 2016

@stevetranby I understand, actually I'd already create a subclass for cocos2d::Menu that does this job, since I don't "feel comfortable" to mess the cocos2dx code in my games. You know, this way I can "just" swap the cocos folder to update the engine (mostly).

But regarding the issue we have a problem in my opnion.
1 - The placement the cocos2d::Menu is wrong (OR the comments are wrong, since they're stating
that Menu is in the center of the screen [line 137])
2 - The call (like you pointed) is superfluous since cocos2d::Layer is already doing that.
3 - The "correct" code i.e. make the cocos2d::Menu centered will break old code (like you pointed too)

What we can do to improve the engine? I mean, "fix the comment" and make the placement of cocos2d::Menu like it is today? Or we put this in the v4 release (I think that is ok, to break this kind
of code in major releases, what do you think?).
Another thing, do you think that is ok to remove the superfluous code?

@ricardoquesada

This comment has been minimized.

Copy link
Contributor

ricardoquesada commented Sep 7, 2016

yes, I confirm that it breaks backwards compatibility.
you can confirm it by just running "cpp-tests". The "stop" menu that should be on the top-right is on the center now.

I wouldn't break backwards compatibility in v4 for this change. It provides tiny benefit for the user, and fix it might take time: "hey all my menus are broken in v4!... how can I fix them".

what superfluous code would you like to remove?

@N2OMatt

This comment has been minimized.

Copy link
Contributor Author

N2OMatt commented Sep 12, 2016

Sorry guys, I was in a vacation last week or so, I didn't check my mail!

@ricardoquesada Ok then, I can see your point ;D
The superflous code is the pointed by @stevetranby in the comment:

"@N2OMatt this will break existing code. Notice that it's actually redundant for Menu to set ignore and anchor as it's already set the same in Layer."

Thanks everyone for all the attention!

@ricardoquesada

This comment has been minimized.

Copy link
Contributor

ricardoquesada commented Sep 13, 2016

closing it as "won't fix" since it breaks compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.