godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.62k stars 21.28k forks source link

Restore rotation_degrees or provide deg_to_rad(Vector*) #63390

Closed TokisanGames closed 1 year ago

TokisanGames commented 2 years ago

Godot version

4.0alpha12+

System information

Win 10/64 NVIDIA GeForce GTX 1060/PCIe/SSE2

Issue description

Core Issues

The engine devs clearly understand that game devs wish to use degrees, because they provide degrees in the inspector. In fact it's the only option. Yet in code we can only work with radians, as rotation_degrees was removed.

It's crazy to use rotation in the inspector with degrees, while rotation in code works in radians. Any experienced game dev is going to be working with rotations in both the inspector and code. Virtually no one is going to use only one.

Radians and degrees are currently both implemented in the engine, so there is absolutely no savings of code or memory by neutering the API. At best, there are a few less lines on the manual page.

This choice has broken our animationplayers, forced us to rewrite our code, and manually convert degrees, and leaves the engine in a weird state where we use two different units for the same variable.

Please give us back the two variable syntactic sugar of rotation(radians) and rotation_degrees. Or if you want to drop one, drop radians. Or find another solution, such as below, but using two different units for rotation is nuts.

Alternative

Rotation, like all transform options are commonly updated frequently, every frame, on potentially thousands of objects. There is a deg_to_rad(float) function, but no Vector* variations.

This means for those of us who want to work in degrees in 3D, we must deconstruct a Vector3, convert each component, then reconstruct the Vector. This is unnecessarily slow in GDScript, every frame, on thousands of objects, when it could be done in C++ (especially since the code is already there).

As an alternative to the above:

Or maybe we do everything: restore rotation_degrees, make the code and inspector function the same way, and provide deg_to_rad(vector) and convert animation tracks. I haven't heard a single good reason for neutering the API. And I've experienced and heard of a lot of pain from devs as we fix things that unnecessarily broke.

Related to #58316, but that's an editor issue.

Steps to reproduce

# Unnecessarily slow:
node.rotation = Vector3( deg_to_rad(new_deg_rotation.x), deg_to_rad(new_deg_rotation.y), deg_to_rad(new_deg_rotation.z) )

# Should be:
node.rotation_degrees = new_deg_rotation
# or
node.rotation = deg_to_rad(new_deg_rotation) 

Temporary Workaround

Fine for limited use, but not optimal for updates on many objects every frame.

# Util.gd
static func d2r(vect: Vector3) -> Vector3:
    return Vector3( deg_to_rad(vect.x), deg_to_rad(vect.y), deg_to_rad(vect.z) )

# Called like:
node.rotation = Util.d2r(new_deg_rotation)
Calinou commented 2 years ago

Out of curiosity, why aren't you working with radians directly within your own code? The TAU constant makes this easy, as 1 tau is equal to 1 full turn (360 degrees).

TokisanGames commented 2 years ago

Perhaps I am projecting, but I believe the general case is that some people will be working with degrees on thousands of objects on a per-frame basis.

This is supported by Godot itself, as the editor only supports degrees and continues to train users to use it. #58316. You already have the conversion currently implemented in the engine in C++. Why not expose it again?

In my current case, I'm porting 30k lines of code and a thousand scenes to Godot 4 (far from automatic). The one class I've come across so far is not a per-frame usage. It is a weapon offset that takes degrees from a user variable and applies it upon equiping. In my player script, I'm working with Basis. In my enemy scripts I think we're mostly working with rotation_y. So I've got different usages all over and I've only just started scraping the surface. I'm mostly working in radians, but every hard-coded usage has to have a comment to translate it into degrees because radians suck. eg. if angle >= 2.6: # 150 degrees

logzero commented 2 years ago

if angle >= 2.6: # 150 degrees

How many such magic numbers do you have in your code. Might be worth defining them somewhere, give them proper names. What does 150 degrees mean here?

TokisanGames commented 2 years ago

When the player is idling and the input direction is greater than 150, she plays a 180 degree turning animation before walking. When the input direction is less than that but greater than 70, she plays a 90 degree turning animation.

There are hardcoded thresholds all over. Slope distance, delays, focal length, etc. Most are consts or export var if multiple use. Single use may or may not get a const.

fire commented 2 years ago

I support creating deg2rad that works on real_t, vector 2, vector 3 and vector 4.

TokisanGames commented 2 years ago

I ran into another issue with this today. Because rotation_degrees was available in 3, I created an animation that uses it for at least two assets. Now those animations are broken and need to be recreated.

Zireael07 commented 2 years ago

@tinmanjuggernaut Same issue here, no conversion it seems :(

sketchyfun commented 2 years ago

Old animations that use rotation_degrees (I have a lot of those in my game) are also broken and left unconverted. I'd definitely like to see rotation_degrees brought back.

Calinou commented 2 years ago

Old animations that use rotation_degrees (I have a lot of those in my game) are also broken and left unconverted. I'd definitely like to see rotation_degrees brought back.

Instead of readding a legacy property, I think the better solution is to make the 3to4 converter handle rotation_degrees animation tracks.

cc @qarmin

akien-mga commented 2 years ago

Old animations that use rotation_degrees (I have a lot of those in my game) are also broken and left unconverted. I'd definitely like to see rotation_degrees brought back.

Instead of readding a legacy property, I think the better solution is to make the 3to4 converter handle rotation_degrees animation tracks.

I agree that the property conversion could/should be done.

But there's still merit in discussing whether the current situation for using degrees in script can be improved. I was never really convinced by @reduz's strong belief that we should expose degrees in the inspector ("for artists") and radians in code ("for programmers") and that somehow handling it all automagically would be the best situation.

Adding more flexibility to deg2rad and making sure that it doesn't incur a significant performance cost would be a good option. Or readding a script-only rotation_degrees indeed, though the main issue here is that there are many other properties which take radians and adding proxies for all of them is a bit of a hassle and documentation bloat.

Zireael07 commented 2 years ago

Animations are the most egregious case being broken currently, but I find myself using degrees in code fairly often (especially for things that are exported, since it's easier to change 50 to 60 than to remember however many radians that is...)

TokisanGames commented 2 years ago

rotation_degrees is literally already implemented in the engine. https://github.com/godotengine/godot/issues/58316 It only needs to be exposed. There is no code savings by not having it as you're already providing it to inspector users, but not animationplayer users or coders.

Providing both for artists and coders is a good choice. Providing only one depending on the interface, or handling things automagically is the worst choice. How many of you enjoy it when Windows or Apple products make choices for you or try to be smarter than you?

I think extending deg2rad would be worth doing, and would hardly take any code, but I understand if it isn't wanted. It's probably not necessary with rotation_degrees exposed.

Putting an animation player conversion in the conversion tool is nice in theory, but it's useless if the tool is not bulletproof. I tried to use it only to find it broke immediately and converted nothing. I'm now using a modified version of Aaron's script for renames and manually converting and testing everything a section at a time. I find that a standalone script is far superior, because it's adjustable in realtime, and I can use it to convert only certain directories or files at a time. And I can revert changes with git and reconvert as I make adjustments to the script or other testing. Working with an immature, built in compiled tool that's only updated on every alpha release doesn't help me at all. I think you guys should consider going the script route since you've made so many changes to the engine and haven't identified them all.

TokisanGames commented 2 years ago

I ran into another case of this today. I have meshes that I've visually adjusted rotations in the editor. I took those settings and hard coded them as defaults and clamped values in code. But in GD4 the editor uses only degrees, and the code only uses radians. So on top of copying one square at a time, I also have to manually convert degrees to radians. Doesn't that seem like a crazy process?

Also discussing with one of my experienced 3D artists today and he didn't even know what radians are. I expect many other new coders will be in the same situation. Not supporting degrees in code is a big mistake.

Out of curiosity, why aren't you working with radians directly within your own code? The TAU constant makes this easy, as 1 tau is equal to 1 full turn (360 degrees).

Tau doesn't help with this. It's a manual conversion. e.g.

    eye_r.rotation_degrees = Vector3(-37, -25, -23)
    eye_l.rotation_degrees = Vector3(-37, 25, 23)
        ...
    eye_r.rotation_degrees.x = clamp(eye_r.rotation_degrees.x, -45, -13)
    eye_r.rotation_degrees.y = clamp(eye_r.rotation_degrees.y, -75, 7)
Zireael07 commented 2 years ago

When I updated my racer game to beta 1, I had to replace around a hundred occurrences of deg2rad and only a couple of rad2deg... I find degrees easier to work with, and I suppose many other users also do - I understand not having duplicated properties but we need easily accessible conversions.

TokisanGames commented 2 years ago

The thing is degree rotation is already in the engine. In the inspector you CANNOT work with radians, only degrees. In code, you CANNOT work with degrees, only radians.

Then our AnimationPlayer rotation tracks broke. But when you remake them, the track values are exactly the same since the editor has always and continues to work in degrees only. It could have just renamed the track.

I have rewritten the ticket above to clarify and prioritize the core issues.

Zireael07 commented 2 years ago

But when you remake them, the track values are exactly the same since the editor has always and continues to work in degrees only. It could have just renamed the track.

Seriously? I have the same issue and this is the bleep solution?! And the converter somehow cannot handle this?

in the inspector you cannot work with radians

I believe it's no longer the case in 4.x, you can switch inspector to radians

TokisanGames commented 2 years ago

Seriously? I have the same issue and this is the bleep solution?! And the converter somehow cannot handle this?

In the editor the values are identical, but the track name is different. I looked at the scene files. In GD3 the animation is rotation_degrees from 0 to -359.9. In GD4 the animation is rotation from 0 to -6.28144. A script could easily handle this. However the better solution is to reenable rotation_degrees.

I believe it's no longer the case in 4.x, you can switch inspector to radians

I built master today. I don't see any option in the inspector under Node3D, Tooltip, Editor settings, or Project settings (searching rotation, degree, or radian). If it's there, it's far from obvious.

The inspector provides options for working in Euler degrees, Quaternions, or Basis. But not Radians. Really? A gamedev is going to manually enter a Basis or a Quaternion in the editor? Or keyframe them (lol)? But they can't use radians in the editor, and a coder can't use degrees? Come on.

Calinou commented 2 years ago

I believe it's no longer the case in 4.x, you can switch inspector to radians

This was proposed in https://github.com/godotengine/godot/pull/50042 but was rejected.

akien-mga commented 2 years ago

I believe it's no longer the case in 4.x, you can switch inspector to radians

This was proposed in #50042 but was rejected.

That's not the same proposal. #50042 was about making all units in the engine configurable. Making degrees/radians configurable is still a goal of the initial implementation and does not require the level of flexibility #50042 advocated for (which is what was rejected).

sketchyfun commented 2 years ago

But when you remake them, the track values are exactly the same since the editor has always and continues to work in degrees only. It could have just renamed the track.

Seriously? I have the same issue and this is the bleep solution?! And the converter somehow cannot handle this?

I'm not sure it's as simple as renaming the track. When I import an animation from 3.x that used 'rotation_degrees', I can rename it to 'rotation', but the keyframe values are way off in the inspector. However, when you hover over the keyframe, it displays the correct values in a tool tip. It looks like it's converting the degree values as if they were radians, so if your initial 'rotation_degrees' keyframe was (0, 360, 0), the inspector value after renaming from 'rotation_degrees' to 'rotation' will be (0, 20626.5, 0)

image

image

TokisanGames commented 2 years ago

The converter solution would rename rotation_degrees to rotation and multiply all keyed values by 0.1744 (PI/180). Very easy to do in a script.

swift502 commented 2 years ago

This change just seems anti-user. Less user choice, less user convenience.

I'm for bringing it back. IMO conversions make code harder to read, as you have to constantly think about at which line the value is in degrees and at which is it in radians.

The bottom line is:

If this feature never existed we'd all be putting up with conversions like you do in Unity. But these properties proved incredibly useful so it's painful to see them get removed.

rainlizard commented 2 years ago

Where was the Godot proposal for removing rotation_degrees, can someone link it?

aaronfranke commented 2 years ago

@rainlizard It was not a proposal, it was removed in #50009 (btw, the title of the PR is wrong, it's not a "fix", it's a significant change to properties and how they are displayed in the editor).

sketchyfun commented 2 years ago

It feels like something that should have had some public discussion before removing, honestly. @reduz can you at least consider the possibility of re-adding it?

StoreBoughtRocketGames commented 2 years ago

Seriously, why was this removed? Radians are unintuitive, and degrees are not. It seems like a no brainer to have rotation_degrees like before. It's a little concerning how cavalier you guys are about changing API's for no good reason.

swift502 commented 1 year ago

Looks like the property will be re-added 👍

https://twitter.com/reduzio/status/1604412578536513538

The amount of users wanting them back is very significant, so they will be re-added (but hidden from the inspector this time)

swift502 commented 1 year ago

70263 addresses this issue.

We can close this now. (or after it's merged)

TokisanGames commented 1 year ago

We can close this now. (or after it's merged)

It will be closed as part of the merging process, not before.