-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Add PackedVector4Array
Variant type
#85474
Conversation
b6359dc
to
32ec12f
Compare
32ec12f
to
26ac16c
Compare
Unfortunately swapping current |
c4a697b
to
d2590f5
Compare
9ac6de8
to
45e3191
Compare
1adbb54
to
b989023
Compare
b989023
to
60f90ed
Compare
We had an argument about Key Points from Lyuma:
Key Points from iFire:
Conclusion:Both participants highlight various aspects of type compatibility, precision, and practical use cases within Godot's engine. Lyuma seems more inclined toward simplifying things without necessarily enforcing double precision for The discussion centers around the implications of introducing a separate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an unresolved comment about asking reduz about bumping the version, but I am ok either way.
Ah yes thanks for the reminder, I'll check that with reduz. |
Missed here, was recently added in #82952: godot/modules/gdscript/gdscript_parser.cpp Lines 4126 to 4133 in 4e9543d
|
Also here: diff --git a/core/io/marshalls.cpp b/core/io/marshalls.cpp
index 03677c6649..a9ffa15130 100644
--- a/core/io/marshalls.cpp
+++ b/core/io/marshalls.cpp
@@ -1327,6 +1327,7 @@ Error encode_variant(const Variant &p_variant, uint8_t *r_buffer, int &r_len, bo
case Variant::VECTOR4:
case Variant::PACKED_VECTOR2_ARRAY:
case Variant::PACKED_VECTOR3_ARRAY:
+ case Variant::PACKED_VECTOR4_ARRAY:
case Variant::TRANSFORM2D:
case Variant::TRANSFORM3D:
case Variant::PROJECTION: And possibly here: diff --git a/modules/mono/editor/bindings_generator.cpp b/modules/mono/editor/bindings_generator.cpp
index 9aef338891..ac34f74474 100644
--- a/modules/mono/editor/bindings_generator.cpp
+++ b/modules/mono/editor/bindings_generator.cpp
@@ -330,6 +330,8 @@ String BindingsGenerator::bbcode_to_text(const String &p_bbcode, const TypeInter
output.append("'" BINDINGS_NAMESPACE ".Vector3[]'");
} else if (tag == "PackedColorArray") {
output.append("'" BINDINGS_NAMESPACE ".Color[]'");
+ } else if (tag == "PackedVector4Array") {
+ output.append("'" BINDINGS_NAMESPACE ".Vector4[]'");
} else {
const TypeInterface *target_itype = _get_type_or_null(TypeReference(tag));
@@ -646,6 +648,8 @@ String BindingsGenerator::bbcode_to_xml(const String &p_bbcode, const TypeInterf
xml_output.append("<see cref=\"" BINDINGS_NAMESPACE ".Vector3\"/>[]");
} else if (tag == "PackedColorArray") {
xml_output.append("<see cref=\"" BINDINGS_NAMESPACE ".Color\"/>[]");
+ } else if (tag == "PackedVector4Array") {
+ xml_output.append("<see cref=\"" BINDINGS_NAMESPACE ".Vector4\"/>[]");
} else {
const TypeInterface *target_itype = _get_type_or_null(TypeReference(tag));
diff --git a/modules/mono/editor/bindings_generator.h b/modules/mono/editor/bindings_generator.h
index bb0ba0cb00..fce530a832 100644
--- a/modules/mono/editor/bindings_generator.h
+++ b/modules/mono/editor/bindings_generator.h
@@ -689,7 +689,7 @@ class BindingsGenerator {
StringName type_Vector4i = StaticCString::create("Vector4i");
// Object not included as it must be checked for all derived classes
- static constexpr int nullable_types_count = 18;
+ static constexpr int nullable_types_count = 19;
StringName nullable_types[nullable_types_count] = {
type_String,
type_StringName,
@@ -711,6 +711,7 @@ class BindingsGenerator {
StaticCString::create(_STR(PackedVector2Array)),
StaticCString::create(_STR(PackedVector3Array)),
StaticCString::create(_STR(PackedColorArray)),
+ StaticCString::create(_STR(PackedVector4Array)),
};
bool is_nullable_type(const StringName &p_type) const { |
ef96abe
to
d929f27
Compare
Indeed, thanks both. I had found some of these while doing another thorough review after rebase, but missed the XML stuff in I also found a handful more places where we were missing the PackedVector4Array equivalent (comparing with all PackedVector3Array usage/handling). And I fixed a bug where type hinted PackedVector4Arrays in the Inspector were not properly editable as arrays of Vector4. We were missing to pass the hint string along to Still pending: Evaluating the compatibility breakage in the text and binary resource formats, and see how to handle it gracefully (like we did for PackedByteArray serialization changes). |
With all this work, I was reminded of this quote:
|
d929f27
to
4608dc3
Compare
I checked and indeed the changes would break forward compat in 4.2. I added this diff here to bump the format version for the binary format too (like we did already for the text format when adding the PackedByteArray base64 serialization), while still saving by default in the previous format if possible. diff --git a/core/io/resource_format_binary.cpp b/core/io/resource_format_binary.cpp
index 99cffde082..ab460c5f4c 100644
--- a/core/io/resource_format_binary.cpp
+++ b/core/io/resource_format_binary.cpp
@@ -90,11 +90,12 @@ enum {
OBJECT_EXTERNAL_RESOURCE = 1,
OBJECT_INTERNAL_RESOURCE = 2,
OBJECT_EXTERNAL_RESOURCE_INDEX = 3,
- // Version 2: added 64 bits support for float and int.
- // Version 3: changed nodepath encoding.
- // Version 4: new string ID for ext/subresources, breaks forward compat.
+ // Version 2: Added 64-bit support for float and int.
+ // Version 3: Changed NodePath encoding.
+ // Version 4: New string ID for ext/subresources, breaks forward compat.
// Version 5: Ability to store script class in the header.
- FORMAT_VERSION = 5,
+ // Version 6: Added PackedVector4Array Variant type.
+ FORMAT_VERSION = 6,
FORMAT_VERSION_CAN_RENAME_DEPS = 1,
FORMAT_VERSION_NO_NODEPATH_PROPERTY = 3,
};
diff --git a/scene/resources/resource_format_text.cpp b/scene/resources/resource_format_text.cpp
index f70a00a9f6..2e27ac9198 100644
--- a/scene/resources/resource_format_text.cpp
+++ b/scene/resources/resource_format_text.cpp
@@ -37,11 +37,12 @@
#include "core/object/script_language.h"
#include "core/version.h"
-// Version 2: changed names for Basis, AABB, Vectors, etc.
-// Version 3: new string ID for ext/subresources, breaks forward compat.
-// Version 4: PackedByteArray is now stored as base64 encoded.
-#define FORMAT_VERSION_COMPAT 3
+// Version 2: Changed names for Basis, AABB, Vectors, etc.
+// Version 3: New string ID for ext/subresources, breaks forward compat.
+// Version 4: PackedByteArray can be base64 encoded, and PackedVector4Array was added.
#define FORMAT_VERSION 4
+// For compat, save as version 3 if not using PackedVector4Array or no big PackedByteArray.
+#define FORMAT_VERSION_COMPAT 3
#define BINARY_FORMAT_VERSION 4
@@ -1979,6 +1980,9 @@ void ResourceFormatSaverTextInstance::_find_resources(const Variant &p_variant,
use_compat = false;
}
} break;
+ case Variant::PACKED_VECTOR4_ARRAY: {
+ use_compat = false;
+ } break;
default: {
}
} PR #91486 adds compat code to 4.2.3 (and 4.1.5 once cherry-picked) so that those versions can properly parse scenes and resources using PackedVector4Array. Properties of that type are converted to plain Arrays, and thus on the trip back to 4.3 there's a risk of losing data. diff --git a/node_3d.tscn b/node_3d.tscn
index 64be59a..72aa829 100644
--- a/node_3d.tscn
+++ b/node_3d.tscn
@@ -1,4 +1,4 @@
-[gd_scene load_steps=4 format=4 uid="uid://bs0waftols4bk"]
+[gd_scene load_steps=4 format=3 uid="uid://bs0waftols4bk"]
[ext_resource type="Texture2D" uid="uid://4siemblwxumr" path="res://icon.svg" id="1_r5ffe"]
[ext_resource type="Script" path="res://Sprite3D.gd" id="2_u0x6a"]
@@ -15,13 +15,13 @@ mesh = SubResource("BoxMesh_ku6f7")
transform = Transform3D(2, 0, 0, 0, 2, 0, 0, 0, 2, 0, 2, 0)
texture = ExtResource("1_r5ffe")
script = ExtResource("2_u0x6a")
-pba1 = PackedByteArray("MnsQF/8slhgAAAAAAAB7ewADAIsAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA")
+pba1 = PackedByteArray(50, 123, 16, 23, 255, 44, 150, 24, 0, 0, 0, 0, 0, 0, 123, 123, 0, 3, 0, 139, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
pba2 = [51, 123, 15]
pv3a2 = [Vector3(1, 1, 1), Vector3(2, 2, 2), Vector3(3, 3, 3), null, Vector3(5, 5, 5)]
-pv4a1 = PackedVector4Array(11, 22, 33, 44, 19, 29, 39, 49, -8, -123, 14, 129)
+pv4a1 = [Vector4(11, 22, 33, 44), Vector4(19, 29, 39, 49), Vector4(-8, -123, 14, 129)]
pv4a2 = [Vector4(9, 82, 7, 7), Vector4(1, 1, 1, 1), Vector4(0, 2, 2, 2), Vector4(3, 3, 3, 3)]
pv4a3 = [Vector4(10, 20, 30, 40), Vector4(110, 120, 130, 140), null, Vector4(2, 2.22, 2, 2), Vector4(141, 3123.12, 21390.1, 1.1), Vector4(0, 0, 0, 0), Vector4(3, 3, 3, 3)]
-metadata/meta_pv4a = PackedVector4Array(0, 0, 0, 0, 1, 2, 3, 4, 10, 20, 30, 40, 0, 0, 0, 0, 100, 200.5, 300.8, 400.9)
+metadata/meta_pv4a = [Vector4(0, 0, 0, 0), Vector4(1, 2, 3, 4), Vector4(10, 20, 30, 40), Vector4(0, 0, 0, 0), Vector4(100, 200.5, 300.8, 400.9)]
[node name="Sprite3D2" type="Sprite3D" parent="."]
transform = Transform3D(-8.74228e-08, 0, 2, 0, 2, 0, -2, 0, -8.74228e-08, -3.32284, 2, 0) So On the way back however, those properties will stay Arrays, and some of them can be lost:
Here's how those properties are defined: @export var pba1 : PackedByteArray
@export var pba2 : PackedByteArray = []
@export var pba3 : PackedByteArray = [0, 2, 13, 8, 12, 151, 90, 42]
@export var pv3a2 : PackedVector3Array = []
@export var pv4a1 : PackedVector4Array
@export var pv4a2 : PackedVector4Array = []
@export var pv4a3 : PackedVector4Array = [Vector4(10, 20, 30, 40), Vector4(110, 120, 130, 140)] I think this is a GDScript issue outside the scope of this PR.
I actually went ahead and committed this change earlier today. Needs testing of shader uniforms to confirm it's still working, if not I can roll it back. |
Co-authored-by: A Thousand Ships <[email protected]> Co-authored-by: Rémi Verschelde <[email protected]>
4608dc3
to
f9b4885
Compare
Ok that didn't work, so I reverted this part of the change. Here's a start for a follow-up if someone wants to tackle changing it PackedVector4Array: diff --git a/servers/rendering/shader_language.cpp b/servers/rendering/shader_language.cpp
index 99b3f54379..9231a91732 100644
--- a/servers/rendering/shader_language.cpp
+++ b/servers/rendering/shader_language.cpp
@@ -3962,12 +3962,9 @@ Variant ShaderLanguage::constant_value_to_variant(const Vector<ShaderLanguage::C
}
value = Variant(array);
} else {
- PackedFloat32Array array;
+ PackedVector4Array array;
for (int i = 0; i < array_size; i += 4) {
- array.push_back(p_value[i].real);
- array.push_back(p_value[i + 1].real);
- array.push_back(p_value[i + 2].real);
- array.push_back(p_value[i + 3].real);
+ array.push_back(Vector4(p_value[i].real, p_value[i + 1].real, p_value[i + 2].real, p_value[i + 3].real));
}
value = Variant(array);
}
@@ -4183,7 +4180,7 @@ PropertyInfo ShaderLanguage::uniform_to_property_info(const ShaderNode::Uniform
if (p_uniform.hint == ShaderLanguage::ShaderNode::Uniform::HINT_SOURCE_COLOR) {
pi.type = Variant::PACKED_COLOR_ARRAY;
} else {
- pi.type = Variant::PACKED_FLOAT32_ARRAY;
+ pi.type = Variant::PACKED_VECTOR4_ARRAY;
}
} else {
if (p_uniform.hint == ShaderLanguage::ShaderNode::Uniform::HINT_SOURCE_COLOR) { The implementation should make sure to preserve compatibility for pre-existing shader params. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go now.
Should we get a @godotengine/dotnet look at the added cases above to check if we've missed anything? I just did a search when I found those cases and I'm not familiar with the mono side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might pick up the shader code task, will look once we merge
Edit: Won't be focusing on it at the moment
Thanks to everyone involved! |
Was curious how hard it was.
Fixes: godotengine/godot-proposals#6129
Bugsquad edit: Keywords for easy searching: PackedVector4Array, Vector<Vector4>