Skip to content

Commit

Permalink
Change Node set_name to use StringName
Browse files Browse the repository at this point in the history
  • Loading branch information
aaronfranke committed Aug 25, 2024
1 parent 28a72fa commit 3e3a3eb
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 41 deletions.
50 changes: 26 additions & 24 deletions core/string/ustring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5367,47 +5367,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;
}

Expand Down
2 changes: 2 additions & 0 deletions core/string/ustring.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
7 changes: 7 additions & 0 deletions misc/extension_api_validation/4.3-stable.expected
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,10 @@ Validate extension JSON: Error: Field 'classes/RenderingDevice/methods/draw_list
draw_list_begin added a new optional debug argument called breadcrumb.
There used to be an Array argument as arg #9 initially, then changed to typedarray::RID in 4.1, and finally removed in 4.3.
Since we're adding a new one at the same location, we need to silence those warnings for 4.1 and 4.3.


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.
6 changes: 1 addition & 5 deletions modules/mono/editor/bindings_generator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) + "'.");
}
Expand Down
41 changes: 41 additions & 0 deletions scene/main/node.compat.inc
Original file line number Diff line number Diff line change
@@ -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
21 changes: 15 additions & 6 deletions scene/main/node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
/**************************************************************************/

#include "node.h"
#include "node.compat.inc"

#include "core/config/project_settings.h"
#include "core/io/resource_loader.h"
Expand Down Expand Up @@ -1330,17 +1331,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);
Expand Down
7 changes: 6 additions & 1 deletion scene/main/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,11 @@ class Node : public Object {
GDVIRTUAL1(_unhandled_input, Ref<InputEvent>)
GDVIRTUAL1(_unhandled_key_input, Ref<InputEvent>)

#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.
Expand Down Expand Up @@ -441,7 +446,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;

Expand Down
6 changes: 1 addition & 5 deletions tests/core/object/test_class_db.h
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,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), "'.");
}
}
Expand Down

0 comments on commit 3e3a3eb

Please sign in to comment.