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

Jump to bottom

Removed faulty function update after get_property_list. #63712

Merged
merged 1 commit into from
Aug 6, 2022
Merged

Removed faulty function update after get_property_list. #63712

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
2 changes: 0 additions & 2 deletions core/object/object.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,6 @@ Variant Object::get_indexed(const Vector<StringName> &p_names, bool *r_valid) co

void Object::get_property_list(List<PropertyInfo> *p_list, bool p_reversed) const {
if (script_instance && p_reversed) {
p_list->push_back(PropertyInfo(Variant::NIL, "Script Variables", PROPERTY_HINT_NONE, String(), PROPERTY_USAGE_CATEGORY));
script_instance->get_property_list(p_list);
}

Expand Down Expand Up @@ -503,7 +502,6 @@ void Object::get_property_list(List<PropertyInfo> *p_list, bool p_reversed) cons
}

if (script_instance && !p_reversed) {
p_list->push_back(PropertyInfo(Variant::NIL, "Script Variables", PROPERTY_HINT_NONE, String(), PROPERTY_USAGE_CATEGORY));
script_instance->get_property_list(p_list);
}

Expand Down
25 changes: 25 additions & 0 deletions core/object/script_language.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,31 @@ Dictionary Script::_get_script_constant_map() {
return ret;
}

#ifdef TOOLS_ENABLED

PropertyInfo Script::get_class_category() const {
String path = get_path();
String name;

if (is_built_in()) {
if (get_name().is_empty()) {
name = TTR("Built-in script");
} else {
name = vformat("%s (%s)", get_name(), TTR("Built-in"));
}
} else {
if (get_name().is_empty()) {
name = path.get_file();
} else {
name = get_name();
}
}

return PropertyInfo(Variant::NIL, name, PROPERTY_HINT_NONE, path, PROPERTY_USAGE_CATEGORY);
}

#endif // TOOLS_ENABLED

void Script::_bind_methods() {
ClassDB::bind_method(D_METHOD("can_instantiate"), &Script::can_instantiate);
//ClassDB::bind_method(D_METHOD("instance_create","base_object"),&Script::instance_create);
Expand Down
1 change: 1 addition & 0 deletions core/object/script_language.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ class Script : public Resource {

#ifdef TOOLS_ENABLED
virtual Vector<DocData::ClassDoc> get_documentation() const = 0;
virtual PropertyInfo get_class_category() const;
#endif // TOOLS_ENABLED

virtual bool has_method(const StringName &p_method) const = 0;
Expand Down
8 changes: 8 additions & 0 deletions core/object/script_language_extension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,6 +663,14 @@ class ScriptInstanceExtension : public ScriptInstance {
if (native_info->get_property_list_func) {
uint32_t pcount;
const GDNativePropertyInfo *pinfo = native_info->get_property_list_func(instance, &pcount);

#ifdef TOOLS_ENABLED
Ref<Script> script = get_script();
if (script->is_valid() && pcount > 0) {
Copy link
Contributor

@Gallilus Gallilus Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @object71 do you remember wy you use x->valid() in stead off x.valid()?
-> makes my GDExtension crash so before making a PR I want to check if it was intentional.

Copy link
Contributor

@Zylann Zylann Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it might need both (Ref::is_valid and Script::is_valid). The first checks if the object actually has a script attached to it. The second checks if the script itself is in a valid state. The code is currently missing the first.

Copy link
Member

@kleonc kleonc Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It indeed should be script.is_valid() as it's the reference which needs to be validated before dereferencing it down below in script->get_class_category().

script.is_valid() will call:

godot/core/object/ref_counted.h

Line 201 in a83eb16

inline bool is_valid() const { return reference != nullptr; }

script->is_valid() will dereference the stored reference pointer (possibly nullptr):

godot/core/object/ref_counted.h

Lines 104 to 106 in a83eb16

_FORCE_INLINE_ T *operator->() const {
return reference;
}

and call (possibly crashing):

godot/core/object/script_language.h

Line 150 in a83eb16

virtual bool is_valid() const = 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "possibly crashing" ?

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "possibly crashing" ?

@Gallilus I mean if the Ref<Script> script's privateScript *reference member is indeed nullptr then script->is_valid() is equivalent to nullptr->is_valid(). And dereferencing nullptr is an undefined behavior, might result in a crash.

Hence before any script->... calls it should be ensured (e.g. by calling script.is_valid()) that it can be dereferenced.

Copy link
Contributor

Choose a reason for hiding this comment

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

This may sound silly but I'm not jet a regular in C++ conversations.
With dereference you mean going true the pointer in the object to do stuff.
Not deleting it from memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I had a .ptr() in my code so ...

    MyClass my_object = memnew(MyClass);
    Ref<MyClass > reference_1 = *my_object;
    Ref<MyClass > reference_2a = reference_1;
    Ref<MyClass > reference_2b = reference_1.ptr();
    if (refrence_2b.is_valid()){
        // Is reference_2 a or b valid?
        // Will reference.is_valid() && refrence->is_valid() catch this problem?
    }

p_list->push_back(script->get_class_category());
}
#endif // TOOLS_ENABLED

for (uint32_t i = 0; i < pcount; i++) {
p_list->push_back(PropertyInfo(pinfo[i]));
}
Expand Down
98 changes: 8 additions & 90 deletions editor/editor_inspector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2539,7 +2539,6 @@ void EditorInspector::update_tree() {

List<PropertyInfo> plist;
object->get_property_list(&plist, true);
_update_script_class_properties(*object, plist);

HashMap<VBoxContainer *, HashMap<String, VBoxContainer *>> vbox_per_path;
HashMap<String, EditorInspectorArray *> editor_inspector_array_per_prefix;
Expand Down Expand Up @@ -2624,18 +2623,24 @@ void EditorInspector::update_tree() {
category_vbox = nullptr; //reset

String type = p.name;
String label = p.name;
type_name = p.name;

// Set the category icon.
if (!ClassDB::class_exists(type) && !ScriptServer::is_global_class(type) && p.hint_string.length() && FileAccess::exists(p.hint_string)) {
// If we have a category inside a script, search for the first script with a valid icon.
Ref<Script> script = ResourceLoader::load(p.hint_string, "Script");
StringName base_type;
StringName name;
if (script.is_valid()) {
base_type = script->get_instance_base_type();
name = EditorNode::get_editor_data().script_class_get_name(script->get_path());
if (name != StringName() && label != name) {
label = name;
}
}
while (script.is_valid()) {
StringName name = EditorNode::get_editor_data().script_class_get_name(script->get_path());
name = EditorNode::get_editor_data().script_class_get_name(script->get_path());
String icon_path = EditorNode::get_editor_data().script_class_get_icon_path(name);
if (name != StringName() && icon_path.length()) {
category->icon = ResourceLoader::load(icon_path, "Texture");
Expand All @@ -2654,7 +2659,7 @@ void EditorInspector::update_tree() {
}

// Set the category label.
category->label = type;
category->label = label;

if (use_doc_hints) {
// Sets the category tooltip to show documentation.
Expand Down Expand Up @@ -3759,93 +3764,6 @@ void EditorInspector::_feature_profile_changed() {
update_tree();
}

void EditorInspector::_update_script_class_properties(const Object &p_object, List<PropertyInfo> &r_list) const {
Ref<Script> script = p_object.get_script();
if (script.is_null()) {
return;
}

List<Ref<Script>> classes;

// NodeC -> NodeB -> NodeA
while (script.is_valid()) {
classes.push_front(script);
script = script->get_base_script();
}

if (classes.is_empty()) {
return;
}

// Script Variables -> to insert: NodeC..B..A -> bottom (insert_here)
List<PropertyInfo>::Element *script_variables = nullptr;
List<PropertyInfo>::Element *bottom = nullptr;
List<PropertyInfo>::Element *insert_here = nullptr;
for (List<PropertyInfo>::Element *E = r_list.front(); E; E = E->next()) {
PropertyInfo &pi = E->get();
if (pi.name != "Script Variables") {
continue;
}
script_variables = E;
bottom = r_list.insert_after(script_variables, PropertyInfo());
insert_here = bottom;
break;
}

HashSet<StringName> added;
for (const Ref<Script> &s : classes) {
String path = s->get_path();
String name = EditorNode::get_editor_data().script_class_get_name(path);
if (name.is_empty()) {
if (s->is_built_in()) {
if (s->get_name().is_empty()) {
name = TTR("Built-in script");
} else {
name = vformat("%s (%s)", s->get_name(), TTR("Built-in"));
}
} else {
name = path.get_file();
}
}

List<PropertyInfo> props;
s->get_script_property_list(&props);

// Script Variables -> NodeA -> bottom (insert_here)
List<PropertyInfo>::Element *category = r_list.insert_before(insert_here, PropertyInfo(Variant::NIL, name, PROPERTY_HINT_NONE, path, PROPERTY_USAGE_CATEGORY));

// Script Variables -> NodeA -> A props... -> bottom (insert_here)
for (List<PropertyInfo>::Element *P = props.front(); P; P = P->next()) {
PropertyInfo &pi = P->get();
if (added.has(pi.name)) {
continue;
}
added.insert(pi.name);

r_list.insert_before(insert_here, pi);

List<PropertyInfo>::Element *prop_below = bottom->next();
while (prop_below) {
if (prop_below->get() == pi) {
List<PropertyInfo>::Element *to_delete = prop_below;
prop_below = prop_below->next();
r_list.erase(to_delete);
} else {
prop_below = prop_below->next();
}
}
}

// Script Variables -> NodeA (insert_here) -> A props... -> bottom
insert_here = category;
}

if (script_variables) {
r_list.erase(script_variables);
r_list.erase(bottom);
}
}

void EditorInspector::set_restrict_to_basic_settings(bool p_restrict) {
restrict_to_basic = p_restrict;
update_tree();
Expand Down
1 change: 0 additions & 1 deletion editor/editor_inspector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,6 @@ class EditorInspector : public ScrollContainer {
void _vscroll_changed(double);

void _feature_profile_changed();
void _update_script_class_properties(const Object &p_object, List<PropertyInfo> &r_list) const;

bool _is_property_disabled_by_feature_profile(const StringName &p_property);

Expand Down
42 changes: 30 additions & 12 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,16 +325,23 @@ void GDScript::_get_script_property_list(List<PropertyInfo> *r_list, bool p_incl
for (int i = 0; i < msort.size(); i++) {
props.push_front(sptr->member_info[msort[i].name]);
}

#ifdef TOOLS_ENABLED
r_list->push_back(sptr->get_class_category());
#endif // TOOLS_ENABLED

for (const PropertyInfo &E : props) {
r_list->push_back(E);
}

props.clear();

if (!p_include_base) {
break;
}

sptr = sptr->_base;
}

for (const PropertyInfo &E : props) {
r_list->push_back(E);
}
}

void GDScript::get_script_property_list(List<PropertyInfo> *r_list) const {
Expand Down Expand Up @@ -434,17 +441,17 @@ void GDScript::set_source_code(const String &p_code) {

#ifdef TOOLS_ENABLED
void GDScript::_update_exports_values(HashMap<StringName, Variant> &values, List<PropertyInfo> &propnames) {
if (base_cache.is_valid()) {
base_cache->_update_exports_values(values, propnames);
}

for (const KeyValue<StringName, Variant> &E : member_default_values_cache) {
values[E.key] = E.value;
}

for (const PropertyInfo &E : members_cache) {
propnames.push_back(E);
}

if (base_cache.is_valid()) {
base_cache->_update_exports_values(values, propnames);
}
}

void GDScript::_add_doc(const DocData::ClassDoc &p_inner_class) {
Expand Down Expand Up @@ -703,6 +710,8 @@ bool GDScript::_update_exports(bool *r_err, bool p_recursive_call, PlaceHolderSc
member_default_values_cache.clear();
_signals.clear();

members_cache.push_back(get_class_category());

for (int i = 0; i < c->members.size(); i++) {
const GDScriptParser::ClassNode::Member &member = c->members[i];

Expand All @@ -728,6 +737,9 @@ bool GDScript::_update_exports(bool *r_err, bool p_recursive_call, PlaceHolderSc
}
_signals[member.signal->identifier->name] = parameters_names;
} break;
case GDScriptParser::ClassNode::Member::GROUP: {
members_cache.push_back(member.annotation->export_info);
} break;
default:
break; // Nothing.
}
Expand Down Expand Up @@ -1510,11 +1522,17 @@ void GDScriptInstance::get_property_list(List<PropertyInfo> *p_properties) const
props.push_front(sptr->member_info[msort[i].name]);
}

sptr = sptr->_base;
}
#ifdef TOOLS_ENABLED
p_properties->push_back(sptr->get_class_category());
#endif // TOOLS_ENABLED

for (const PropertyInfo &prop : props) {
p_properties->push_back(prop);
}

props.clear();

for (const PropertyInfo &E : props) {
p_properties->push_back(E);
sptr = sptr->_base;
}
}

Expand Down
Loading