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.

Sign up for GitHub

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

Relaxes node name sanitization in gltf documents. #45545

Merged
merged 1 commit into from
Mar 9, 2021
Merged

Relaxes node name sanitization in gltf documents. #45545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions core/string/ustring.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4364,6 +4364,18 @@ String String::property_name_encode() const {
return *this;
}

// Changes made to the set of invalid characters must also be reflected in the String documentation.
const String String::invalid_node_name_characters = ". : @ / \"";

String String::validate_node_name() const {
Vector<String> chars = String::invalid_node_name_characters.split(" ");
Copy link
Member

@akien-mga akien-mga Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is pre-existing code but I'm not sure if there's much gain having this as a static constant which then gets split() on each call. Compilers might be able to optimize this I guess? But we could just as well have const String restricted_chars = ".:@/\""; inside the function and iterate over the characters directly.

CC @bruvzg if you have a suggestion on how best to do that (don't want to get mixed up with const char * and char32_t :)).

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see invalid_node_name_characters is used in SceneTreeEditor` to show a nice error message. Makes sense then I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same (I'm dubious that this would get memoized by the compiler) but my gut feel is that validate_node_name is probably not called frequently enough for the split to have a meaningful impact on performance in typical scenarios. If there are scenarios where very large numbers of nodes are getting created it'd be trivial to modify the error message case (which should be even more rare), but I wanted to keep changes limited to the original bug as much as practical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Vector<String> chars = String::invalid_node_name_characters.split(" ");
// Changes made to the set of invalid characters must also be reflected in the String documentation.
const String String::invalid_node_name_characters = ".:@/\"";
String String::validate_node_name() const {
String invalid_char(&String::invalid_node_name_characters[0], 1);
String replacement("");
String name = this->replace(invalid_char, replacement);
for (int i = 1; i < String::invalid_node_name_characters.length(); ++i) {
invalid_char[0] = String::invalid_node_name_characters[i];
name = name.replace(invalid_char, replacement);
}
return name;
}

String name = this->replace(chars[0], "");
for (int i = 1; i < chars.size(); i++) {
name = name.replace(chars[i], "");
}
return name;
}

String String::get_basename() const {
int pos = rfind(".");
if (pos < 0 || pos < MAX(rfind("/"), rfind("\\"))) {
Expand Down
4 changes: 4 additions & 0 deletions core/string/ustring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,10 @@ class String {

String property_name_encode() const;

// node functions
static const String invalid_node_name_characters;
String validate_node_name() const;

bool is_valid_identifier() const;
bool is_valid_integer() const;
bool is_valid_float() const;
Expand Down
2 changes: 2 additions & 0 deletions core/variant/variant_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,8 @@ static void _register_variant_builtin_methods() {
bind_method(String, c_unescape, sarray(), varray());
bind_method(String, json_escape, sarray(), varray());

bind_method(String, validate_node_name, sarray(), varray());

bind_method(String, is_valid_identifier, sarray(), varray());
bind_method(String, is_valid_integer, sarray(), varray());
bind_method(String, is_valid_float, sarray(), varray());
Expand Down
7 changes: 7 additions & 0 deletions doc/classes/String.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,6 +887,13 @@
[/codeblocks]
</description>
</method>
<method name="validate_node_name">
<return type="String">
</return>
<description>
Removes any characters from the string that are prohibited in [Node] names ([code].[/code] [code]:[/code] [code]@[/code] [code]/[/code] [code]"[/code]).
</description>
</method>
<method name="xml_escape">
<return type="String">
</return>
Expand Down
8 changes: 5 additions & 3 deletions editor/scene_tree_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,9 +767,11 @@ void SceneTreeEditor::_renamed() {
return;
}

String new_name = which->get_text(0);
if (!Node::_validate_node_name(new_name)) {
error->set_text(TTR("Invalid node name, the following characters are not allowed:") + "\n" + Node::invalid_character);
String raw_new_name = which->get_text(0);
String new_name = raw_new_name.validate_node_name();

if (new_name != raw_new_name) {
error->set_text(TTR("Invalid node name, the following characters are not allowed:") + "\n" + String::invalid_node_name_characters);
error->popup_centered();

if (new_name.is_empty()) {
Expand Down
46 changes: 37 additions & 9 deletions modules/gltf/gltf_document.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
#include "scene/3d/node_3d.h"
#include "scene/3d/skeleton_3d.h"
#include "scene/animation/animation_player.h"
#include "scene/main/node.h"
#include "scene/resources/surface_tool.h"
#include <limits>

Expand Down Expand Up @@ -449,22 +450,16 @@ Error GLTFDocument::_serialize_nodes(Ref<GLTFState> state) {
return OK;
}

String GLTFDocument::_sanitize_scene_name(const String &name) {
RegEx regex("([^a-zA-Z0-9_ -]+)");
String p_name = regex.sub(name, "", true);
return p_name;
}

String GLTFDocument::_gen_unique_name(Ref<GLTFState> state, const String &p_name) {
const String s_name = _sanitize_scene_name(p_name);
const String s_name = p_name.validate_node_name();

String name;
int index = 1;
while (true) {
name = s_name;

if (index > 1) {
name += " " + itos(index);
name += itos(index);
}
if (!state->unique_names.has(name)) {
break;
Expand All @@ -477,6 +472,39 @@ String GLTFDocument::_gen_unique_name(Ref<GLTFState> state, const String &p_name
return name;
}

String GLTFDocument::_sanitize_animation_name(const String &p_name) {
// Animations disallow the normal node invalid characters as well as "," and "["
// (See animation/animation_player.cpp::add_animation)

// TODO: Consider adding invalid_characters or a validate_animation_name to animation_player to mirror Node.
String name = p_name.validate_node_name();
name = name.replace(",", "");
name = name.replace("[", "");
return name;
}

String GLTFDocument::_gen_unique_animation_name(Ref<GLTFState> state, const String &p_name) {
const String s_name = _sanitize_animation_name(p_name);

String name;
int index = 1;
while (true) {
name = s_name;

if (index > 1) {
name += itos(index);
abaire marked this conversation as resolved.
Show resolved Hide resolved
}
if (!state->unique_animation_names.has(name)) {
break;
}
index++;
}

state->unique_animation_names.insert(name);

return name;
}

String GLTFDocument::_sanitize_bone_name(const String &name) {
String p_name = name.camelcase_to_underscore(true);

Expand Down Expand Up @@ -4729,7 +4757,7 @@ Error GLTFDocument::_parse_animations(Ref<GLTFState> state) {
if (name.begins_with("loop") || name.ends_with("loop") || name.begins_with("cycle") || name.ends_with("cycle")) {
animation->set_loop(true);
}
animation->set_name(_sanitize_scene_name(name));
animation->set_name(_gen_unique_animation_name(state, name));
}

for (int j = 0; j < channels.size(); j++) {
Expand Down
3 changes: 2 additions & 1 deletion modules/gltf/gltf_document.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,9 @@ class GLTFDocument : public Resource {
Error _parse_nodes(Ref<GLTFState> state);
String _get_type_name(const GLTFType p_component);
String _get_accessor_type_name(const GLTFDocument::GLTFType p_type);
String _sanitize_scene_name(const String &name);
String _gen_unique_name(Ref<GLTFState> state, const String &p_name);
String _sanitize_animation_name(const String &name);
String _gen_unique_animation_name(Ref<GLTFState> state, const String &p_name);
String _sanitize_bone_name(const String &name);
String _gen_unique_bone_name(Ref<GLTFState> state,
const GLTFSkeletonIndex skel_i,
Expand Down
11 changes: 11 additions & 0 deletions modules/gltf/gltf_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ void GLTFState::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_lights", "lights"), &GLTFState::set_lights);
ClassDB::bind_method(D_METHOD("get_unique_names"), &GLTFState::get_unique_names);
ClassDB::bind_method(D_METHOD("set_unique_names", "unique_names"), &GLTFState::set_unique_names);
ClassDB::bind_method(D_METHOD("get_unique_animation_names"), &GLTFState::get_unique_animation_names);
ClassDB::bind_method(D_METHOD("set_unique_animation_names", "unique_animation_names"), &GLTFState::set_unique_animation_names);
ClassDB::bind_method(D_METHOD("get_skeletons"), &GLTFState::get_skeletons);
ClassDB::bind_method(D_METHOD("set_skeletons", "skeletons"), &GLTFState::set_skeletons);
ClassDB::bind_method(D_METHOD("get_skeleton_to_node"), &GLTFState::get_skeleton_to_node);
Expand Down Expand Up @@ -98,6 +100,7 @@ void GLTFState::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "cameras", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_cameras", "get_cameras"); // Vector<Ref<GLTFCamera>>
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "lights", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_lights", "get_lights"); // Vector<Ref<GLTFLight>>
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "unique_names", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_unique_names", "get_unique_names"); // Set<String>
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "unique_animation_names", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_unique_animation_names", "get_unique_animation_names"); // Set<String>
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "skeletons", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_skeletons", "get_skeletons"); // Vector<Ref<GLTFSkeleton>>
ADD_PROPERTY(PropertyInfo(Variant::DICTIONARY, "skeleton_to_node", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_skeleton_to_node", "get_skeleton_to_node"); // Map<GLTFSkeletonIndex,
ADD_PROPERTY(PropertyInfo(Variant::ARRAY, "animations", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_INTERNAL | PROPERTY_USAGE_EDITOR), "set_animations", "get_animations"); // Vector<Ref<GLTFAnimation>>
Expand Down Expand Up @@ -255,6 +258,14 @@ void GLTFState::set_unique_names(Array p_unique_names) {
GLTFDocument::set_from_array(unique_names, p_unique_names);
}

Array GLTFState::get_unique_animation_names() {
return GLTFDocument::to_array(unique_animation_names);
}

void GLTFState::set_unique_animation_names(Array p_unique_animation_names) {
GLTFDocument::set_from_array(unique_animation_names, p_unique_animation_names);
}

Array GLTFState::get_skeletons() {
return GLTFDocument::to_array(skeletons);
}
Expand Down
4 changes: 4 additions & 0 deletions modules/gltf/gltf_state.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class GLTFState : public Resource {
Vector<Ref<GLTFCamera>> cameras;
Vector<Ref<GLTFLight>> lights;
Set<String> unique_names;
Set<String> unique_animation_names;

Vector<Ref<GLTFSkeleton>> skeletons;
Map<GLTFSkeletonIndex, GLTFNodeIndex> skeleton_to_node;
Expand Down Expand Up @@ -147,6 +148,9 @@ class GLTFState : public Resource {
Array get_unique_names();
void set_unique_names(Array p_unique_names);

Array get_unique_animation_names();
void set_unique_animation_names(Array p_unique_names);

Array get_skeletons();
void set_skeletons(Array p_skeletons);

Expand Down
16 changes: 1 addition & 15 deletions scene/main/node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,22 +981,8 @@ void Node::_set_name_nocheck(const StringName &p_name) {
data.name = p_name;
}

String Node::invalid_character = ". : @ / \"";

bool Node::_validate_node_name(String &p_name) {
String name = p_name;
Vector<String> chars = Node::invalid_character.split(" ");
for (int i = 0; i < chars.size(); i++) {
name = name.replace(chars[i], "");
}
bool is_valid = name == p_name;
p_name = name;
return is_valid;
}

void Node::set_name(const String &p_name) {
String name = p_name;
_validate_node_name(name);
String name = p_name.validate_node_name();

ERR_FAIL_COND(name == "");
data.name = name;
Expand Down
6 changes: 0 additions & 6 deletions scene/main/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,6 @@ class Node : public Object {

void _set_tree(SceneTree *p_tree);

#ifdef TOOLS_ENABLED
friend class SceneTreeEditor;
#endif
static String invalid_character;
static bool _validate_node_name(String &p_name);

protected:
void _block() { data.blocked++; }
void _unblock() { data.blocked--; }
Expand Down
14 changes: 14 additions & 0 deletions tests/test_string.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,20 @@ TEST_CASE("[String] humanize_size") {
CHECK(String::humanize_size(100523550) == "95.86 MiB");
CHECK(String::humanize_size(5345555000) == "4.97 GiB");
}

TEST_CASE("[String] validate_node_name") {
String numeric_only = "12345";
CHECK(numeric_only.validate_node_name() == "12345");

String name_with_spaces = "Name with spaces";
CHECK(name_with_spaces.validate_node_name() == "Name with spaces");

String name_with_kana = "Name with kana ゴドツ";
CHECK(name_with_kana.validate_node_name() == "Name with kana ゴドツ");

String name_with_invalid_chars = "Name with invalid characters :.@removed!";
CHECK(name_with_invalid_chars.validate_node_name() == "Name with invalid characters removed!");
}
} // namespace TestString

#endif // TEST_STRING_H