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

[Core] Reformat structure string operators #34668

Merged
merged 1 commit into from
Jun 13, 2021

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Dec 29, 2019

Implements and closes godotengine/godot-proposals#1819

Explanation in bullets below each set of outputs. Test code:

func _ready():
	print("Vector2")
	print(Vector2())
	print("Vector2i")
	print(Vector2i())
	print("Vector3")
	print(Vector3())
	print("Vector3i")
	print(Vector3i())
	print("Color")
	print(Color())
	print("Quaternion")
	print(Quaternion())
	print("Plane")
	print(Plane())
	print("AABB")
	print(AABB())
	print("Rect2")
	print(Rect2())
	print("Rect2i")
	print(Rect2i())
	var t = Transform2D()
	t.x = Vector2(1, 5)
	print("Transform2D")
	print(t)
	var b = Basis()
	b.x = Vector3(1, 5, 0)
	print("Basis")
	print(b)
	print("Transform3D")
	print(Transform(b))

Output with current master:

Vector2
(0, 0)
Vector2i
(0, 0)
Vector3
(0, 0, 0)
Vector3i
(0, 0, 0)
Color
0,0,0,1
Quaternion
(0, 0, 0, 1)
Plane
0, 0, 0, 0
AABB
0, 0, 0 - 0, 0, 0
Rect2
(0, 0, 0, 0)
Rect2i
(0, 0, 0, 0)
Transform2D
((1, 5), (0, 1), (0, 0))
Basis
((1, 0, 0), (5, 1, 0), (0, 0, 1))
Transform3D
1, 5, 0, 0, 1, 0, 0, 0, 1 - 0, 0, 0

The current output is a bit confusing and inconsistent:

  • Color is lacking spaces after the commas.

  • AABB and Transform both contain a dash, which can be mistaken for a minus sign.

  • Some things have parenthesis, some don't, and Basis/Transform2D have nested parenthesis.

  • Transform2D prints out column-major, while Basis prints out row-major, and Transform is row major except the origin is last.

  • The current internal code is a bit strange, with some of the string logic being inside of Variant and some being in the class.

Output with this PR:

Vector2
(0, 0)
Vector2i
(0, 0)
Vector3
(0, 0, 0)
Vector3i
(0, 0, 0)
Color
(0, 0, 0, 1)
Quaternion
(0, 0, 0, 1)
Plane
[N: (0, 0, 0), D: 0]
AABB
[P: (0, 0, 0), S: (0, 0, 0)]
Rect2
[P: (0, 0), S: (0, 0)]
Rect2i
[P: (0, 0), S: (0, 0)]
Transform2D
[X: (1, 5), Y: (0, 1), O: (0, 0)]
Basis
[X: (1, 5, 0), Y: (0, 1, 0), Z: (0, 0, 1)]
Transform3D
[X: (1, 5, 0), Y: (0, 1, 0), Z: (0, 0, 1), O: (0, 0, 0)]
  • I've moved all the code to the classes and out of Variant.

  • No dashes anywhere, parenthesis used for grouping with commas for separation.

  • Transforms and Basis are now printed consistently and with explicit labels to avoid any confusion.

  • Since they store real_t, I decided to use String::num_real, but normally this adds a trailing .0 to whole numbers, so I added a new bool to include it or not called p_trailing, which defaults to true so its current uses are the same.

  • Color is an exception to the above, it uses String::num with a fixed amount of decimals (4 seems like a good value, but maybe 5, 3 or 2 would be better).

@aaronfranke
Copy link
Member Author

aaronfranke commented Feb 17, 2020

I updated this PR with C# changes that match the behavior above.

This should be good to merge any time, the sooner the better.

core/ustring.cpp Outdated
@@ -1325,8 +1325,10 @@ String String::num_real(double p_num) {
dec_int /= 10;
}
sd = '.' + decimal;
} else {
} else if (p_trailing) {
Copy link
Member

@akien-mga akien-mga Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to make this optional though? IMO it would be just as well to enforce it, so that it's always clear that it's a real and not an integer.

Edit: I'm not asking to change it for now, just putting this up for debate.

Copy link
Member Author

@aaronfranke aaronfranke Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either is fine to me. I made it this way because it's the existing behavior for floats, but maybe it's better to always show it to make it clear that it's a float.

P.S. In C#, the default behavior of converting numbers to string does not include the trailing decimals, so if we want them I'd add a format specifier ("0.0#############").

EDIT: Actually, the C# bindings generator currently depends on it not having trailing decimals. This could be fixed in the bindings generator, though.

@aaronfranke aaronfranke force-pushed the to-string branch 2 times, most recently from af9f0d5 to e69c3b6 Compare May 15, 2020 07:27
@aaronfranke aaronfranke force-pushed the to-string branch 3 times, most recently from a4bae90 to 392a061 Compare July 24, 2020 23:40
@aaronfranke aaronfranke force-pushed the to-string branch 2 times, most recently from 9353c1d to 6100b5c Compare August 5, 2020 01:50
@jonbonazza
Copy link
Contributor

@akien-mga @aaronfranke i put this in the accompanying proposal already, but this is important so im leaving it here too. This can't be merged as-is

Printing mtiple lines is npt an option. Often times, you want to coalesce the string representation into another string. Most importantly though, it makes it unusable in log lines (for instance in server logs that are consumed by a log consumer such as fluentd)

The order of numbers is not changed except for Transform2D. All logic is done inside of their structures (and not in Variant).

For the number of decimals printed, they now use String::num_real which works best with real_t, except for Color which is fixed at 4 decimals (this is a reliable number of float digits when converting from 16-bpc so it seems like a good choice)
@akien-mga akien-mga merged commit 600b4c9 into godotengine:master Jun 13, 2021
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the to-string branch June 13, 2021 15:29
@RolandMarchand
Copy link

Legendary PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reformat structure string operators
6 participants