From 9a8cbb613170976b170d4745d0410832fd54a1e0 Mon Sep 17 00:00:00 2001 From: Aaron Franke Date: Fri, 28 Apr 2023 15:59:00 -0500 Subject: [PATCH] Change Node set_name to use StringName --- core/string/ustring.cpp | 50 ++++++++++--------- core/string/ustring.h | 2 + .../4.1-stable_4.2-stable.expected | 9 +--- .../4.2-stable.expected | 7 +++ modules/mono/editor/bindings_generator.cpp | 6 +-- scene/main/node.compat.inc | 41 +++++++++++++++ scene/main/node.cpp | 21 +++++--- scene/main/node.h | 7 ++- tests/core/object/test_class_db.h | 6 +-- 9 files changed, 101 insertions(+), 48 deletions(-) create mode 100644 scene/main/node.compat.inc diff --git a/core/string/ustring.cpp b/core/string/ustring.cpp index 3d37e17ef836..aaf178a26cc8 100644 --- a/core/string/ustring.cpp +++ b/core/string/ustring.cpp @@ -5227,47 +5227,49 @@ String String::get_invalid_node_name_characters(bool p_allow_internal) { return r; } -String String::validate_node_name() const { - // This is a critical validation in node addition, so it must be optimized. - const char32_t *cn = ptr(); - if (cn == nullptr) { - return String(); +int32_t String::invalid_node_name_index() const { + const char32_t *string_chars_ptr = ptr(); + if (string_chars_ptr == nullptr) { + return -1; } - bool valid = true; - uint32_t idx = 0; - while (cn[idx]) { - const char32_t *c = invalid_node_name_characters; - while (*c) { - if (cn[idx] == *c) { - valid = false; - break; + uint32_t index = 0; + while (string_chars_ptr[index]) { + const char32_t *invalid_chars_ptr = invalid_node_name_characters; + while (*invalid_chars_ptr) { + if (string_chars_ptr[index] == *invalid_chars_ptr) { + return index; } - c++; + invalid_chars_ptr++; } - if (!valid) { - break; - } - idx++; + index++; } + return -1; +} - if (valid) { +String String::validate_node_name() const { + ERR_FAIL_COND_V_MSG(is_empty(), *this, "An empty String is not a valid node name and cannot be validated."); + int32_t invalid_index = invalid_node_name_index(); + if (invalid_index == -1) { return *this; } + return validate_node_name_internal(invalid_index); +} +String String::validate_node_name_internal(int32_t p_index) const { + // This is a critical validation in node addition, so it must be optimized. String validated = *this; char32_t *nn = validated.ptrw(); - while (nn[idx]) { + while (nn[p_index]) { const char32_t *c = invalid_node_name_characters; while (*c) { - if (nn[idx] == *c) { - nn[idx] = '_'; + if (nn[p_index] == *c) { + nn[p_index] = '_'; break; } c++; } - idx++; + p_index++; } - return validated; } diff --git a/core/string/ustring.h b/core/string/ustring.h index 9df2d56e8004..e11ea2d21d26 100644 --- a/core/string/ustring.h +++ b/core/string/ustring.h @@ -458,7 +458,9 @@ class String { // node functions static String get_invalid_node_name_characters(bool p_allow_internal = false); + int32_t invalid_node_name_index() const; String validate_node_name() const; + String validate_node_name_internal(int32_t p_index) const; String validate_identifier() const; String validate_filename() const; diff --git a/misc/extension_api_validation/4.1-stable_4.2-stable.expected b/misc/extension_api_validation/4.1-stable_4.2-stable.expected index 11cf8531c639..4b320e72167e 100644 --- a/misc/extension_api_validation/4.1-stable_4.2-stable.expected +++ b/misc/extension_api_validation/4.1-stable_4.2-stable.expected @@ -1,10 +1,5 @@ -This file contains the expected output of --validate-extension-api when run against the extension_api.json of the -4.1-stable tag (the basename of this file). - -Only lines that start with "Validate extension JSON:" matter, everything else is considered a comment and ignored. They -should instead be used to justify these changes and describe how users should work around these changes. - -Add new entries at the end of the file. +This file contains, when concatenated to the expected output since 4.2, the expected output of --validate-extension-api +when run against the extension_api.json of the 4.1-stable tag (first part of the basename of this file). ## Changes between 4.1-stable and 4.2-stable diff --git a/misc/extension_api_validation/4.2-stable.expected b/misc/extension_api_validation/4.2-stable.expected index ce8f24c7a964..7330deea2193 100644 --- a/misc/extension_api_validation/4.2-stable.expected +++ b/misc/extension_api_validation/4.2-stable.expected @@ -380,3 +380,10 @@ Validate extension JSON: Error: Field 'classes/Image/methods/get_mipmap_offset/r Type changed to int64_t to support baking large lightmaps. No compatibility method needed, both GDExtension and C# generate it as int64_t anyway. + + +GH-76560 +-------- +Validate extension JSON: Error: Field 'classes/Node/methods/set_name/arguments/0': type changed value in new API, from "String" to "StringName". + +Change Node `set_name` to use StringName to improve performance. diff --git a/modules/mono/editor/bindings_generator.cpp b/modules/mono/editor/bindings_generator.cpp index 9a76a256395b..b446273f36d7 100644 --- a/modules/mono/editor/bindings_generator.cpp +++ b/modules/mono/editor/bindings_generator.cpp @@ -2539,11 +2539,7 @@ Error BindingsGenerator::_generate_cs_property(const BindingsGenerator::TypeInte if (getter && setter) { const ArgumentInterface &setter_first_arg = setter->arguments.back()->get(); if (getter->return_type.cname != setter_first_arg.type.cname) { - // Special case for Node::set_name - bool whitelisted = getter->return_type.cname == name_cache.type_StringName && - setter_first_arg.type.cname == name_cache.type_String; - - ERR_FAIL_COND_V_MSG(!whitelisted, ERR_BUG, + ERR_FAIL_V_MSG(ERR_BUG, "Return type from getter doesn't match first argument of setter for property: '" + p_itype.name + "." + String(p_iprop.cname) + "'."); } diff --git a/scene/main/node.compat.inc b/scene/main/node.compat.inc new file mode 100644 index 000000000000..173fc227a49c --- /dev/null +++ b/scene/main/node.compat.inc @@ -0,0 +1,41 @@ +/**************************************************************************/ +/* node.compat.inc */ +/**************************************************************************/ +/* This file is part of: */ +/* GODOT ENGINE */ +/* https://godotengine.org */ +/**************************************************************************/ +/* Copyright (c) 2014-present Godot Engine contributors (see AUTHORS.md). */ +/* Copyright (c) 2007-2014 Juan Linietsky, Ariel Manzur. */ +/* */ +/* Permission is hereby granted, free of charge, to any person obtaining */ +/* a copy of this software and associated documentation files (the */ +/* "Software"), to deal in the Software without restriction, including */ +/* without limitation the rights to use, copy, modify, merge, publish, */ +/* distribute, sublicense, and/or sell copies of the Software, and to */ +/* permit persons to whom the Software is furnished to do so, subject to */ +/* the following conditions: */ +/* */ +/* The above copyright notice and this permission notice shall be */ +/* included in all copies or substantial portions of the Software. */ +/* */ +/* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, */ +/* EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF */ +/* MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. */ +/* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY */ +/* CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, */ +/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */ +/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +/**************************************************************************/ + +#ifndef DISABLE_DEPRECATED + +void Node::_set_name_bind_compat_76560(const String &p_name) { + set_name(p_name); +} + +void Node::_bind_compatibility_methods() { + ClassDB::bind_compatibility_method(D_METHOD("set_name", "name"), &Node::_set_name_bind_compat_76560); +} + +#endif diff --git a/scene/main/node.cpp b/scene/main/node.cpp index 0396f3ab4afa..8aa4c3bf6f6d 100644 --- a/scene/main/node.cpp +++ b/scene/main/node.cpp @@ -29,6 +29,7 @@ /**************************************************************************/ #include "node.h" +#include "node.compat.inc" #include "core/config/project_settings.h" #include "core/io/resource_loader.h" @@ -1304,17 +1305,25 @@ void Node::_set_name_nocheck(const StringName &p_name) { data.name = p_name; } -void Node::set_name(const String &p_name) { +void Node::set_name(const StringName &p_name) { ERR_FAIL_COND_MSG(data.inside_tree && !Thread::is_main_thread(), "Changing the name to nodes inside the SceneTree is only allowed from the main thread. Use `set_name.call_deferred(new_name)`."); - String name = p_name.validate_node_name(); - - ERR_FAIL_COND(name.is_empty()); + StringName new_name; + { + String input_name_str = p_name; + ERR_FAIL_COND(input_name_str.is_empty()); + int32_t invalid_index = input_name_str.invalid_node_name_index(); + if (invalid_index == -1) { + new_name = p_name; + } else { + new_name = input_name_str.validate_node_name_internal(invalid_index); + } + } if (data.unique_name_in_owner && data.owner) { _release_unique_name_in_owner(); } - String old_name = data.name; - data.name = name; + StringName old_name = data.name; + data.name = new_name; if (data.parent) { data.parent->_validate_child_name(this, true); diff --git a/scene/main/node.h b/scene/main/node.h index ee195ddef965..88117fed81e0 100644 --- a/scene/main/node.h +++ b/scene/main/node.h @@ -360,6 +360,11 @@ class Node : public Object { GDVIRTUAL1(_unhandled_input, Ref) GDVIRTUAL1(_unhandled_key_input, Ref) +#ifndef DISABLE_DEPRECATED + void _set_name_bind_compat_76560(const String &p_name); + static void _bind_compatibility_methods(); +#endif + public: enum { // You can make your own, but don't use the same numbers as other notifications in other nodes. @@ -416,7 +421,7 @@ class Node : public Object { StringName get_name() const; String get_description() const; - void set_name(const String &p_name); + void set_name(const StringName &p_name); InternalMode get_internal_mode() const; diff --git a/tests/core/object/test_class_db.h b/tests/core/object/test_class_db.h index c1aa39031df0..ceedf0613af7 100644 --- a/tests/core/object/test_class_db.h +++ b/tests/core/object/test_class_db.h @@ -323,11 +323,7 @@ void validate_property(const Context &p_context, const ExposedClass &p_class, co if (getter && setter) { const ArgumentData &setter_first_arg = setter->arguments.back()->get(); if (getter->return_type.name != setter_first_arg.type.name) { - // Special case for Node::set_name - bool whitelisted = getter->return_type.name == p_context.names_cache.string_name_type && - setter_first_arg.type.name == p_context.names_cache.string_type; - - TEST_FAIL_COND(!whitelisted, + TEST_FAIL( "Return type from getter doesn't match first argument of setter, for property: '", p_class.name, ".", String(p_prop.name), "'."); } }