Removed faulty function update after get_property_list. #63712

object71 · 2022-07-31T08:10:36Z

The function tried to rearrange properties but that lead to problems with duplication or deleted properties. Implemented the logic that that function did inside the get_property_list both for tool scripts and non-tool scripts.

Fixes #63668

There shouldn't be an update function that rearranges properties in the first place. If there was a requirement for properties to be split into categories based on the class inheritance this should've initially been implemented into GDScript or GDScriptInstance function for getting properties.

NOT TESTED on GDExtension and C# scripts but it should work.

core/object/script_language.cpp

modules/mono/csharp_script.cpp

object71 · 2022-07-31T20:03:42Z

I cannot compile for mono, can anybody confirm that c# scripts look okay innthe inspector both for rool an non tool scripts with inheritance?

raulsntos · 2022-07-31T21:19:28Z

To compile mono you need to rebase ~~and apply PR #63537 because currently the mono module is broken in master~~ (Edit: the PR was merged and the mono module should be working in master now). With those changes master should compile, but your PR still has an issue, it can be fixed with this patch:

diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 0e3c17557c..62463e787a 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -1806,7 +1806,6 @@ void CSharpInstance::get_property_list(List<PropertyInfo> *p_properties) const {
 	}
 
 #ifdef TOOLS_ENABLED
-	Ref<Script> script = get_script();
 	if (script->is_valid()) {
 		props.push_front(script->get_class_category());
 	}

This is probably what you intended to do, you were shadowing the script member with get_script which just returns it with a less specific type so we can't access the script_class and native members later. With those changes this PR seems to be working fine.

object71 · 2022-08-01T04:34:10Z

To compile mono you need to rebase ~~and apply PR #63537 because currently the mono module is broken in master~~ (Edit: the PR was merged and the mono module should be working in master now). With those changes master should compile, but your PR still has an issue, it can be fixed with this patch:
diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 0e3c17557c..62463e787a 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -1806,7 +1806,6 @@ void CSharpInstance::get_property_list(List<PropertyInfo> *p_properties) const {
 	}
 
 #ifdef TOOLS_ENABLED
-	Ref<Script> script = get_script();
 	if (script->is_valid()) {
 		props.push_front(script->get_class_category());
 	}
This is probably what you intended to do, you were shadowing the script member with get_script which just returns it with a less specific type so we can't access the script_class and native members later. With those changes this PR seems to be working fine.

Thanks, will fix that one!

object71 · 2022-08-01T06:52:32Z

Managed to get mono running and fixed the inspector for mono too. It now looks good both for inherited, tool and non-tool scripts. :)

raulsntos · 2022-08-01T14:16:26Z

Awesome, thank you for adding categories to mono 👍

It seems the changes from push_front to push_back are causing the exported properties in mono to be reversed now:

GDScript	C#

object71 · 2022-08-01T16:55:40Z

Ah, ok. Will fix the ordering.

object71 · 2022-08-03T05:11:12Z

Reveresed the properties addition for C#.

lufog · 2022-08-03T14:45:58Z

Checked on the artifact from actions, @export_category(...), @export_group(...) and @export_subgroup(...) do not work.

More

Code:

@export_category("Category 1")
@export_group("Group A", "group_a")
@export var group_a_var_a: float
@export var group_a_var_b: float
@export_group("Group B", "group_b")
@export var group_b_var_a: float
@export var group_b_var_b: float
@export_group("Group C", "group_c")
@export_subgroup("Subgroup A", "group_c_a")
@export var group_c_a_var_a: float
@export var group_c_a_var_b: float
@export_subgroup("Subgroup B", "group_c_b")
@export var group_c_b_var_a: float
@export var group_c_b_var_b: float

Expected:

Current:

object71 · 2022-08-03T15:01:57Z

Thanks for the note. I forgot to test those new features. Will see to it.

YuriSizov · 2022-08-03T15:04:24Z

Checked on the artifact from actions, @export_category(...), @export_group(...) and @export_subgroup(...) do not work.

They seem to only work in tool scripts now. Which is very weird, because they are parsed together with other annotations.

Double-checked, and it works correctly in alpha 13 and current master.

Edit: I guess in non-tool scripts script-defined categories and groups are stripped for some reason? And _update_script_class_properties re-introduced them by happenstance? That would also explain why it was unnoticed for the old-school way to define groups with _get_property_list, as that requires a tool script to begin with.

YuriSizov · 2022-08-03T16:18:27Z

@object71 I figured how to fix the grouping annotations properly, you need to add a case in GDScript::_update_exports() to include them in member cache.

Something like this seems to be enough:

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

		switch (member.type) {
			// ...

			case GDScriptParser::ClassNode::Member::GROUP: {
				members_cache.push_back(member.annotation->export_info);
			} break;
			default:
				break; // Nothing.
		}
	}

Unlike regular variable members they don't have default values, but this doesn't seem to crash anything. Worst case scenario, some usages of member_default_values_cache may need to be guarded.

YuriSizov · 2022-08-03T17:30:39Z

I've tested some chaotic stuff with the aforementioned fix, and it seems to work as expected. For example, defining a category at the top of your script would hide the base script category, effectively overriding it. Pretty sweet!

godot.windows.tools.64_2022-08-03_20-26-40.mp4

There is a bug, though — when you change the exports on the base class, the inspector for the extending class does not react to these changes. You need to reload the scene to see the changes properly. This is probably outside of the scope for this particular PR, however.

Code looks way better now, and IMO makes sense. I'd like a review from @vnen on this.

modules/gdscript/gdscript.cpp

The function tried to rearrange properties but that lead to problems with duplication or deleted properties. Implemented the logic that that function did inside the get_property_list both for tool scripts and non-tool scripts.

object71 · 2022-08-03T18:47:04Z

Thanks for the great suggestions and fixes. I added them in the latest version. Yeah I noticed the thing where it requires a reload of the scene to catch up some variables but it seems to be due to no call to update for the cached variables and I think it would be out of the scope of this PR. It is complicated enough as it is.

akien-mga · 2022-08-06T11:23:09Z

I want this properly reviewed by @reduz but I expect this might take time to merge given the scope of the changes.

In the meantime, I was wondering if we should revert #58443 to fix the arguably "worse" regression in master / possibly alpha 14, until this new PR gets merged (and any changes from #58443 which are still needed can be added back here). WDYT?

YuriSizov · 2022-08-06T12:14:24Z

OR! We could merge it for alpha 14 and let the public testing commence 😛

Up to you, but either way we have a rather prominent issue(s), whether we keep it as is or revert it.

akien-mga · 2022-08-06T12:16:06Z

Well I wouldn't merge a core change without reduz even looking at it. Especially not as a follow-up to another core change which I merged without his review, which he then said was hacky, and which introduced a regression :)

YuriSizov · 2022-08-06T12:17:53Z

To be fair, he said it was okay to merge it, just needed comments, and he called the method itself hacky, and the method was added in 2020, and I agree that it's a big hack.

akien-mga · 2022-08-06T12:27:13Z

Well that's settled 😄

akien-mga · 2022-08-06T12:27:20Z

Thanks!

gb2dev · 2022-08-06T12:29:54Z

Can someone check if this breaks #62936?

YuriSizov · 2022-08-06T12:34:02Z

@bfeber I don't see why it would, the logic related to type_name which is used for the docs doesn't seem to be touched here.

gb2dev · 2022-08-06T13:41:19Z

It does break it, just tested and compared the GitHub Actions builds.

YuriSizov · 2022-08-06T15:19:11Z

For future reference, the discussion continued in #63997.

Gallilus · 2023-06-17T19:45:33Z

core/object/script_language_extension.h

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


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.

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.

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;

What do you mean by "possibly crashing" ?

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.

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.

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? }

object71 requested review from a team as code owners July 31, 2022 08:10

object71 force-pushed the fix-export-issues branch from 8bc06ad to 0dfffe9 Compare July 31, 2022 08:17

akien-mga reviewed Jul 31, 2022

View reviewed changes

core/object/script_language.cpp Outdated Show resolved Hide resolved

object71 force-pushed the fix-export-issues branch 5 times, most recently from f87320d to a677eae Compare July 31, 2022 12:15

raulsntos reviewed Jul 31, 2022

View reviewed changes

modules/mono/csharp_script.cpp Outdated Show resolved Hide resolved

Calinou added bug topic:editor regression labels Jul 31, 2022

Calinou added this to the 4.0 milestone Jul 31, 2022

object71 force-pushed the fix-export-issues branch 4 times, most recently from 1838a40 to 92675e9 Compare August 1, 2022 06:52

object71 force-pushed the fix-export-issues branch from 92675e9 to 0ad894e Compare August 1, 2022 07:33

stylemistake mentioned this pull request Aug 2, 2022

Exported variable appears doubled in object inspector #63668

Closed

object71 force-pushed the fix-export-issues branch from 0ad894e to 589c4fd Compare August 3, 2022 05:09

YuriSizov approved these changes Aug 3, 2022

View reviewed changes

modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved

modules/gdscript/gdscript.cpp Outdated Show resolved Hide resolved

Removed faulty function update after get_property_list.

0e1f7e9

The function tried to rearrange properties but that lead to problems with duplication or deleted properties. Implemented the logic that that function did inside the get_property_list both for tool scripts and non-tool scripts.

object71 force-pushed the fix-export-issues branch from a3038d4 to 0e1f7e9 Compare August 3, 2022 18:47

akien-mga added the topic:core label Aug 4, 2022

reduz approved these changes Aug 6, 2022

View reviewed changes

akien-mga merged commit 77d3ac7 into godotengine:master Aug 6, 2022

neikeq mentioned this pull request Aug 9, 2022

Merge .NET 6 branch with master #64089

Merged

object71 deleted the fix-export-issues branch August 18, 2022 12:04

raulsntos mentioned this pull request Aug 28, 2022

Improve a property list's mapping to internal organization of scripts for better Inspector UX. godotengine/godot-proposals#2803

Closed

kleonc mentioned this pull request Oct 13, 2022

Child resource class initialized before parent resource class #67334

Open

YuriSizov mentioned this pull request Nov 18, 2022

[Godot 4] Object.get_property_list() and Script.get_script_property_list() contain Category, Group and SubGroup. #68780

Open

Gallilus reviewed Jun 17, 2023

View reviewed changes

This was referenced Jun 18, 2023

Fix Ref<>.is_valid() for ScriptInstanceExtension #78392

Merged

ScriptInstanceExtension GDExtensionObjectPtr is no longer readable on Godot Side #78116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed faulty function update after get_property_list. #63712

Removed faulty function update after get_property_list. #63712

object71 commented Jul 31, 2022 •

edited

Loading

object71 commented Jul 31, 2022

raulsntos commented Jul 31, 2022 •

edited

Loading

object71 commented Aug 1, 2022

object71 commented Aug 1, 2022

raulsntos commented Aug 1, 2022

object71 commented Aug 1, 2022

object71 commented Aug 3, 2022

lufog commented Aug 3, 2022 •

edited

Loading

object71 commented Aug 3, 2022

YuriSizov commented Aug 3, 2022 •

edited

Loading

YuriSizov commented Aug 3, 2022 •

edited

Loading

YuriSizov commented Aug 3, 2022

object71 commented Aug 3, 2022

akien-mga commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

akien-mga commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

akien-mga commented Aug 6, 2022

akien-mga commented Aug 6, 2022

gb2dev commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

gb2dev commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

Gallilus Jun 17, 2023 •

edited

Loading

Zylann Jun 17, 2023 •

edited

Loading

kleonc Jun 17, 2023 •

edited

Loading

Gallilus Jun 18, 2023

kleonc Jun 18, 2023

Gallilus Jun 18, 2023

Gallilus Jun 18, 2023

Removed faulty function update after get_property_list. #63712

Removed faulty function update after get_property_list. #63712

Conversation

object71 commented Jul 31, 2022 • edited Loading

object71 commented Jul 31, 2022

raulsntos commented Jul 31, 2022 • edited Loading

object71 commented Aug 1, 2022

object71 commented Aug 1, 2022

raulsntos commented Aug 1, 2022

object71 commented Aug 1, 2022

object71 commented Aug 3, 2022

lufog commented Aug 3, 2022 • edited Loading

object71 commented Aug 3, 2022

YuriSizov commented Aug 3, 2022 • edited Loading

YuriSizov commented Aug 3, 2022 • edited Loading

YuriSizov commented Aug 3, 2022

object71 commented Aug 3, 2022

akien-mga commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

akien-mga commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

akien-mga commented Aug 6, 2022

akien-mga commented Aug 6, 2022

gb2dev commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

gb2dev commented Aug 6, 2022

YuriSizov commented Aug 6, 2022

Gallilus Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Zylann Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

kleonc Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Gallilus Jun 18, 2023

Choose a reason for hiding this comment

kleonc Jun 18, 2023

Choose a reason for hiding this comment

Gallilus Jun 18, 2023

Choose a reason for hiding this comment

Gallilus Jun 18, 2023

Choose a reason for hiding this comment

object71 commented Jul 31, 2022 •

edited

Loading

raulsntos commented Jul 31, 2022 •

edited

Loading

lufog commented Aug 3, 2022 •

edited

Loading

YuriSizov commented Aug 3, 2022 •

edited

Loading

YuriSizov commented Aug 3, 2022 •

edited

Loading

Gallilus Jun 17, 2023 •

edited

Loading

Zylann Jun 17, 2023 •

edited

Loading

kleonc Jun 17, 2023 •

edited

Loading