godotengine / godot

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

Surprising edge cases with export suffix hints #95844

Open tetrapod00 opened 3 weeks ago

tetrapod00 commented 3 weeks ago

Tested versions

4.3

System information

Windows 10

Issue description

Noting some edge cases around export suffix/unit hints. Test script:

extends Node
@export_category("Possibly nonsensical")
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _basis : Basis
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _quaternion : Quaternion

@export_category("Missing fields (Also possibly nonsensical)")
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _transform2d : Transform2D
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _transform3d : Transform3D
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _plane : Plane
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _projection : Projection

@export_category("Supported and Sensible")
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _float : float
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _int : int
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec2 : Vector2
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec2I : Vector2i
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec3 : Vector3
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec3I : Vector3i
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec4 : Vector4
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _vec4I : Vector4i
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _rect2 : Rect2
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _aabb : AABB

@export_category("Not supported (Not Numeric)")
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _bool : bool
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _color : Color
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _string : String
@export_custom(PROPERTY_HINT_NONE, "suffix:units") var _nodepath : NodePath

Output: Godot_v4 3-stable_win64_JErXtxIgGA Godot_v4 3-stable_win64_RG1etM5qC3

Surprising Behavior: Missing field suffixes: Transform2D, Transform3D, Plane, and Projection only have unit suffixes on the last input field in each row. Other multiline types like Rect2 and AABB do have every field suffixed. Strings are not able to be suffixed. (I assume this was outside of the original scope of adding unit suffixes to numeric types; but as a user it is something I want anyway)

Suffixes for unitless or mixed-unit types: Transform2D, Transform3D, Plane, Projection, Basis, and Quaternion arguably do not make sense to have units - they have defined meanings for each field, and mostly are unitless (my math may be off for some of these). I don't think it makes sense to restrict the use of the suffix export, though. Types can be reused for different purposes.

Possible Fix: String should arguably be able to be suffixed. Transform2D, Transform3D, Plane, and Projection should have suffixes on each subfield.

Steps to reproduce

Open the MRP and select the only node in the scene. Look at its inspector.

Minimal reproduction project (MRP)

mrp-suffix-hints.zip

RedMser commented 3 weeks ago

Most of what you wrote makes sense, but there's one correction to make:

Transform2D, Transform3D, Plane, and Projection should have suffixes on each subfield

Regarding Transform2D, Transform3D, Plane: The fields that are suffixed are position data (a 2d or 3d point, or in case of Plane the distance from origin). The ones that are not suffixed are factors which get multiplied (or in case of Plane it's a direction vector), which makes no sense to prefix with a unit. This is also why the Scale property of nodes does not have a unit. So I think the current behavior makes more sense, should not be changed.

Projection I'm not sure how it is used, so I don't know how units should work there.

tetrapod00 commented 3 weeks ago

So for the other two "maybe nonsensical" cases:

Going back to Transform3D, though. Aren't the currently unlabeled fields of a Transform3D the basis, with the labeled fields the Vector3 origin? And Basis is composed of 3 Vector3s, which would use the same units (default meters) as the Vector3 origin? When I wrote the original post, I thought that the basis part of the transform made no sense to have units since it was a rotation (among other things), but now I've convinced myself that the whole thing uses the same units (meters) by default and could be suffixed if the user is using it for their own math.

I'm working off of only half-remembered linear algebra, working knowledge of rotations in games, and Godot's own docs on Basis and Transform3D, so I probably still have some misconceptions about this.