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

Restore rotation_degrees or provide deg_to_rad(Vector*) #63390

Closed
TokisanGames opened this issue Jul 24, 2022 · 29 comments · Fixed by #70263
Closed

Restore rotation_degrees or provide deg_to_rad(Vector*) #63390

TokisanGames opened this issue Jul 24, 2022 · 29 comments · Fixed by #70263

Comments

@TokisanGames
Copy link
Contributor

TokisanGames commented Jul 24, 2022

Godot version

4.0alpha12+

System information

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

Issue description

Core Issues

  • Many of us used rotation_degrees in our code and AnimationPlayers. It was removed for no good reason. Please restore it.

  • The Inspector and code use different units for the same rotation variable. (?!)

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
Copy link
Member

Calinou commented Jul 24, 2022

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
Copy link
Contributor Author

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
Copy link

logzero commented Jul 24, 2022

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
Copy link
Contributor Author

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
Copy link
Member

fire commented Jul 24, 2022

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

@TokisanGames
Copy link
Contributor Author

TokisanGames commented Jul 26, 2022

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
Copy link
Contributor

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

@sketchyfun
Copy link
Contributor

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
Copy link
Member

Calinou commented Jul 28, 2022

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

rotation_degrees is literally already implemented in the engine. #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
Copy link
Contributor Author

TokisanGames commented Aug 13, 2022

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
Copy link
Contributor

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 TokisanGames changed the title Restore rotation_degrees or add deg2rad(Vector2/3/4) Restore rotation_degrees Sep 26, 2022
@TokisanGames TokisanGames changed the title Restore rotation_degrees Restore rotation_degrees or provide deg_to_rad(Vector*) Sep 26, 2022
@TokisanGames
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

Calinou commented Sep 26, 2022

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.

@akien-mga
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

swift502 commented Sep 30, 2022

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:

  • 360 is more readable than Mathf.Tau
  • 75 is more readable than Mathf.DegToRad(75)

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
Copy link
Contributor

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

@aaronfranke
Copy link
Member

@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
Copy link
Contributor

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
Copy link

StoreBoughtRocketGames commented Nov 27, 2022

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
Copy link

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)

@Calinou Calinou added this to the 4.0 milestone Dec 18, 2022
@Calinou Calinou moved this to To Assess in 4.x Priority Issues Dec 18, 2022
@swift502
Copy link

swift502 commented Dec 18, 2022

#70263 addresses this issue.

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

@TokisanGames
Copy link
Contributor Author

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

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

@kleonc kleonc linked a pull request Dec 18, 2022 that will close this issue
@kleonc kleonc moved this from To Assess to In Progress in 4.x Priority Issues Dec 18, 2022
Repository owner moved this from In Progress to Done in 4.x Priority Issues Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.