Replace radians range hint with radians_as_degrees
#82195
Conversation
AThousandShips
force-pushed
the
radian_fix
branch
from
0ccc8a9
to
e4a2028
Compare
|
If this gets merged I'll look into uses of the now deprecated hint elsewhere in documentation etc. |
|
(Missed a documentation entry in |
AThousandShips
changed the title
radians with radians_as_degreesradians range hint with radians_as_degrees
AThousandShips
force-pushed
the
radian_fix
branch
from
e4a2028
to
56c1ef1
Compare
There was a problem hiding this comment.
I prefer the current name. Marking a property as "radians" means "this property uses radians as its unit", which is an independent thing from how the editor decides to convert the unit. For example, if we ever add an editor setting to display radians as radians, the property hint "radians_as_degrees" makes no sense.
|
But the range is confusing with the naming, and no such system is in place, and are there even any plans to do so? And how would that be useful in the interface? The current phrasing, without deep knowledge of the engine, is confusing |
That doesn't work when the other part of the export hint, the range itself, doesn't use radians as its unit. And we can't change that without breaking compatibility. So we need a new name regardless, even if we want to introduce the behavior that you describe in #73170 (comment). I agree that "radians_as_degrees" caters to one specific behavior, but that's our current and only behavior. And it also aligns the name with how the rest of the hint is set. If we ever add a way to display this value in some other format, it's going to be even a bigger mess, because the actual property would be in radians, the hinted range would be in degrees, and the displayed value would be in ???. We shouldn't restrict our solutions based on some potential use case in the future, especially given that that use case is unlikely to fit the current implementation. |
|
Also, while I don't think there's any really good reason to allow editing radians in the inspector, renaming it now would free up |
|
Renaming it should at least indicate that it doesn't do a radians-equivalent of the Might I suggest using 'to' instead of 'as' so as to match the conversion functions ( Also, it's not that important but you can already export radians -- it's just exporting a float normally? |
Already discussed and rejected as it implies the value is modified as opposed to just represented differently |
AThousandShips
force-pushed
the
radian_fix
branch
from
56c1ef1
to
e45927b
Compare
| #ifdef DISABLE_DEPRECATED | ||
| bool is_angle = prop_info.type == Variant::FLOAT && prop_info.hint_string.find("radians_as_degrees") != -1; | ||
| #else | ||
| bool is_angle = prop_info.type == Variant::FLOAT && prop_info.hint_string.find("radians") != -1; | ||
| #endif // DISABLE_DEPRECATED |
There was a problem hiding this comment.
You need to test both in the case where DISABLE_DEPRECATED is not defined. Supporting deprecated syntax doesn't mean not supporting the new syntax, it means supporting both (new first, and fallback to deprecated).
There was a problem hiding this comment.
Well prop_info.hint_string.find("radians") != -1 is true if prop_info.hint_string.find("radians_as_degrees") != -1
I originally added both but then realized that one is superfluous
There was a problem hiding this comment.
Granted it will allow anything including radians but I'm not sure how to test cleanly here for whole words, and the full phrase would also accept things like aaaaradians_as_degreesaaaa
|
Thanks! |
|
Thank you! |
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
`radians_as_degrees` was added in Godot [4.2][1] to replace `radians` with identical functionality and a better name. `suffix` was added in [4.0][2] and was seemingly missed when `#[export(range)]` was first implemented. This is the first instance where Godot has deprecated an API and gdext has needed to implement it. Hopefully this will serve as a useful template for future work. This also fixes a previously unknown bug where a `KEY=VALUE` expression in the third slot of `range = (...)` would yield strange error messages. [1]: godotengine/godot#82195 [2]: godotengine/godot#50009
Deprecates the old syntax
Added compatibility details to the areas I can find that checks for the old tag, haven't attempted any auto-conversion internally or in relation to 3.x, and couldn't find any references to it in mono
If there are any areas that depend on the original syntax I think they can be solved at a later time as it is not pressing
Also renamed the member variable in the editor property classes to make it clear what they do and align with the annotation
As per #73170 (comment) and RocketChat