Skip to content

CCRects converted with jsb does not create origin/size properties #4010

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

CCRects converted with jsb does not create origin/size properties #4010

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions cocos/scripting/javascript/bindings/ScriptingCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,10 +1799,9 @@ jsval ccacceleration_to_jsval(JSContext* cx, const Acceleration& v) {
jsval ccrect_to_jsval(JSContext* cx, const Rect& v) {
JSObject *tmp = JS_NewObject(cx, NULL, NULL, NULL);
if (!tmp) return JSVAL_NULL;
JSBool ok = JS_DefineProperty(cx, tmp, "x", DOUBLE_TO_JSVAL(v.origin.x), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT) &&
JS_DefineProperty(cx, tmp, "y", DOUBLE_TO_JSVAL(v.origin.y), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT) &&
JS_DefineProperty(cx, tmp, "width", DOUBLE_TO_JSVAL(v.size.width), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT) &&
JS_DefineProperty(cx, tmp, "height", DOUBLE_TO_JSVAL(v.size.height), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT);
JSBool ok = JS_DefineProperty(cx, tmp, "size", ccsize_to_jsval(cx, v.size), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT) &&
JS_DefineProperty(cx, tmp, "origin", ccpoint_to_jsval(cx, v.origin), NULL, NULL, JSPROP_ENUMERATE | JSPROP_PERMANENT);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @luisparravicini ,
We should not delete x, y, width, height directly and use origin, size . That will break the compatibility of JSB.
Do you have any ideas about keep the compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping x/y/height/width and adding size/origin would be exactly like what getBoundingBox() returns in js.

The thing is, (maybe because this is my first attempt at modifying cocos2d and not knowing the inner workings of it) but I don't really know why a ccrect in this case is not using size/origin.

Can you take a look at my last note on http://www.cocos2d-x.org/issues/3065#note-3 ?
Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the API point of view, I think it is simpler to use x, y, width, height directly.
Although origin and size are correct, the user has to type more.

So, instead of fixing CCRect and I would fix the return value getBoundingBox()

I do not know if that can be done automatically with the bindings... if the return value is CCRect, then return x,y,width,height.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm doing too many questions: if CCRects (when used through jsb) have to return x, y, width, height, which is what's happening now (not taking into account this pr), why not leave getBoundingBox() as is but change in js cc._RectApplyAffineTransformIn to just don't return origin and size? That's what made me create the issue and this patch in the first place (unless such a change in the API is unacceptable).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Luis.
Tomorrow I'll review our code... I'll give you a detailed answer tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this bug report about regarding this:
http://www.cocos2d-x.org/issues/3260

if (ok) {
return OBJECT_TO_JSVAL(tmp);
}
Expand Down