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

set_rot() is in degree in editor but in radian in script #4511

Closed
ghost opened this issue May 1, 2016 · 9 comments
Closed

set_rot() is in degree in editor but in radian in script #4511

ghost opened this issue May 1, 2016 · 9 comments

Comments

@ghost
Copy link

ghost commented May 1, 2016

Operating system or device:
Windows 10 Pro

Issue description (what happened, and what was expected):
When setting rot from script you must use deg2rad, but in editor it's in degree. It was supposed to be either everything in radians or in degrees. At the very least something in the docs saying that it was in radian.

Steps to reproduce:
Attach a new script to a sprite and write set_rot(80). It's going to be completely erroneous. Now put set_rot(deg2rad(80)). It works just fine.

@rgrams
Copy link

rgrams commented May 1, 2016

+1. This inconsistency has annoyed me quite a bit. Everything should be the same, presumably in degrees.

@mattiascibien
Copy link
Contributor

mattiascibien commented May 4, 2016

I guess this is the offending piece of code:

void Node2D::set_rot(float p_angle) {
    if (_xform_dirty)
        ((Node2D*)this)->_update_xform_values();
    angle=p_angle; //this line
    _update_transform();
    _change_notify("transform/rot");
}

Found here

If needed I can fix it pretty easily i think but I want to ask the devs first to ensure that I am in the right place.

EDIT: i gess that this should also be updated:

float Node2D::get_rot() const {
    if (_xform_dirty)
        ((Node2D*)this)->_update_xform_values();

    return angle;
}

@akien-mga
Copy link
Member

I guess this is the offending piece of code:

The issue is not here, set_rot is defined to use radians in the whole code base, which is pretty normal when doing mathematic operations. In Node2D, you have _set_rotd for degrees.

The problem is here:

ADD_PROPERTYNZ(PropertyInfo(Variant::REAL,"transform/rot",PROPERTY_HINT_RANGE,"-1440,1440,0.1"),_SCS("_set_rotd"),_SCS("_get_rotd"));

The transform/rot parameter uses the _set_rotd method, so expects degrees. But when switching to GDScript, people use set_rot which expects radians.

Not sure how to fix that without breaking all existing Godot games :)

@mattiascibien
Copy link
Contributor

I guess I read the issue all wrong. Sorry.

Not sure how to fix that without breaking all existing Godot games

exactly what I was thinking.

@reduz
Copy link
Member

reduz commented May 4, 2016

This behavior is intended, Godot works with radians internally, but will
show degrees when presented to the user via the UI.

On Wed, May 4, 2016 at 9:41 AM, Mattias Cibien notifications@github.com
wrote:

I guess I read the issue all wrong. Sorry.

Not sure how to fix that without breaking all existing Godot games

exactly what I was thinking.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#4511 (comment)

@akien-mga
Copy link
Member

As I mentioned on Facebook:

We could maybe expose set_rotd to GDScript, and rename the arguments to "radians" and "degrees" respectively so that the autocompletion makes it obvious.

Will implement that.

@akien-mga akien-mga self-assigned this May 6, 2016
@blurymind
Copy link

@akien-mga thank you. Sorry for the inconvenience.

@akien-mga
Copy link
Member

No problem, I do agree that this apparent inconsistency is confusing, even if it has a good justification as @reduz mentioned.

akien-mga added a commit to akien-mga/godot that referenced this issue May 6, 2016
Made public the various set/getters for rotations in degrees.
For consistency, renamed the exposed method names to remove the leading
underscore, and kept the old names with a deprecation warning.

Fixes godotengine#4511.
@blurymind
Copy link

Thank you !

akien-mga added a commit that referenced this issue May 12, 2016
Made public the various set/getters for rotations in degrees.
For consistency, renamed the exposed method names to remove the leading
underscore, and kept the old names with a deprecation warning.

Fixes #4511.

(cherry picked from commit 4eab767)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants