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.

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

Crash with Godot 4 when setting value with "@tool" #61660

Closed
AurelienCaille opened this issue Jun 3, 2022 · 10 comments
Closed

Crash with Godot 4 when setting value with "@tool" #61660

AurelienCaille opened this issue Jun 3, 2022 · 10 comments

Comments

@AurelienCaille
Copy link

Godot version

Godot 4 alpha 9

System information

Windows 10

Issue description

Godot 4 editor crash when setting value with "@tool" and "set"

Steps to reproduce

  • open "my_new_node_3d.tscn"
  • try to set a new color
  • editor crash

Minimal reproduction project

CrashGodot4Setter.zip

@AurelienCaille AurelienCaille changed the title Crash with Godot 4 when setting value Crash with Godot 4 when setting value with "@tool" Jun 3, 2022
@Calinou Calinou added this to the 4.0 milestone Jun 3, 2022
@Chaosus
Copy link
Member

Chaosus commented Jun 3, 2022

I didn't observe a crash in this particular case, but sometimes it crashes when saving scripts, indeed.

@AurelienCaille
Copy link
Author

2022-06-03.16-46-45_Trim.mp4

instant crash when value is setted

@Chaosus
Copy link
Member

Chaosus commented Jun 3, 2022

Yeah, there is a crash. I catch the following log with VisualStudio:

	godot.windows.opt.tools.64.exe!GDScriptInstance::callp(const StringName & p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 1543	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::callp(const StringName & p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 817	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Variant::callp(const StringName & p_method, const Variant * * p_args, int p_argcount, Variant & r_ret, Callable::CallError & r_error) Line 1021	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!GDScriptFunction::call(GDScriptInstance * p_instance, const Variant * * p_args, int p_argcount, Callable::CallError & r_err, GDScriptFunction::CallState * p_state) Line 1544	C++	Symbols loaded.

repeated a million times after

 godot.windows.opt.tools.64.exe!GDScriptInstance::callp(const StringName & p_method, const Variant * * p_args, int p_argcount, Callable::CallError & r_error) Line 1543	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!GDScriptInstance::set(const StringName & p_name, const Variant & p_value) Line 1291	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::set(const StringName & p_name, const Variant & p_value, bool * r_valid) Line 380	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!UndoRedo::_process_operation_list(List<UndoRedo::Operation,DefaultAllocator>::Element * E) Line 343	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!UndoRedo::_redo(bool p_execute) Line 77	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!UndoRedo::commit_action(bool p_execute) Line 297	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!EditorInspector::_edit_set(const String & p_name, const Variant & p_value, bool p_refresh_all, const String & p_changed_field) Line 3312	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!EditorInspector::_property_changed(const String & p_path, const Variant & p_value, const String & p_name, bool p_changing, bool p_update_all) Line 3332	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!call_with_variant_args_helper<EditorInspector,String const &,Variant const &,String const &,bool,bool,0,1,2,3,4>(EditorInspector * p_instance, void(EditorInspector::*)(const String &, const Variant &, const String &, bool, bool) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0,1,2,3,4> __formal) Line 236	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args(EditorInspector *) Line 350	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!CallableCustomMethodPointer<EditorInspector,String const &,Variant const &,String const &,bool,bool>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Callable::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 51	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1120	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!EditorProperty::emit_changed(const StringName & p_property, const Variant & p_value, const StringName & p_field, bool p_changing) Line 118	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!EditorPropertyColor::_color_changed(const Color & p_color) Line 2738	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args_helper(EditorPropertyColor *) Line 236	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args(EditorPropertyColor * p_instance, void(EditorPropertyColor::*)(const Color &) p_method, const Variant * *) Line 350	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!CallableCustomMethodPointer<EditorPropertyColor,Color const &>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Callable::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 51	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1120	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!Object::emit_signal(const StringName &) Line 773	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!ColorPickerButton::_color_changed(const Color & p_color) Line 1294	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!call_with_variant_args_helper<ColorPickerButton,Color const &,0>(ColorPickerButton * p_instance, void(ColorPickerButton::*)(const Color &) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0> __formal) Line 241	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args(ColorPickerButton *) Line 350	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!CallableCustomMethodPointer<ColorPickerButton,Color const &>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Callable::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 51	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1120	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::emit_signal<Color>(const StringName & p_name, Color <p_args_0>) Line 773	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!ColorPicker::_uv_input(const Ref<InputEvent> & p_event, Control * c) Line 876	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!call_with_variant_args_helper<ColorPicker,Ref<InputEvent> const &,Control *,0,1>(ColorPicker * p_instance, void(ColorPicker::*)(const Ref<InputEvent> &, Control *) p_method, const Variant * * p_args, Callable::CallError & r_error, IndexSequence<0,1> __formal) Line 236	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args(ColorPicker *) Line 350	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!CallableCustomMethodPointer<ColorPicker,Ref<InputEvent> const &,Control *>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 105	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Callable::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 51	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Object::emit_signalp(const StringName & p_name, const Variant * * p_args, int p_argcount) Line 1120	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!Object::emit_signal(const StringName &) Line 773	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Control::_call_gui_input(const Ref<InputEvent> & p_event) Line 931	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Viewport::_gui_call_input(Control * p_control, const Ref<InputEvent> & p_input) Line 1302	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Viewport::_gui_input_event(Ref<InputEvent> p_event) Line 1537	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Viewport::push_input(const Ref<InputEvent> & p_event, bool p_local_coords) Line 2742	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Window::_window_input(const Ref<InputEvent> & p_ev) Line 974	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args_helper(Window *) Line 236	C++	Symbols loaded.
 	[Inline Frame] godot.windows.opt.tools.64.exe!call_with_variant_args(Window * p_instance, void(Window::*)(const Ref<InputEvent> &) p_method, const Variant * *) Line 350	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!CallableCustomMethodPointer<Window,Ref<InputEvent> const &>::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 104	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Callable::call(const Variant * * p_arguments, int p_argcount, Variant & r_return_value, Callable::CallError & r_call_error) Line 51	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!DisplayServerWindows::_dispatch_input_event(const Ref<InputEvent> & p_event) Line 2077	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Input::_parse_input_event_impl(const Ref<InputEvent> & p_event, bool p_is_emulated) Line 658	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!Input::flush_buffered_events() Line 884	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!DisplayServerWindows::process_events() Line 1784	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!OS_Windows::run() Line 777	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!widechar_main(int argc, wchar_t * * argv) Line 175	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!_main() Line 199	C++	Symbols loaded.
 	godot.windows.opt.tools.64.exe!main(int argc, char * * argv) Line 211	C++	Symbols loaded.
 	[External Code]		Annotated Frame

Seems like an infinite recursion with the following stack overflow.

@TokisanGames
Copy link
Contributor

TokisanGames commented Jul 3, 2022

This is caused by a pretty significant change I just discovered in Godot 4 from Godot 3. In Godot 4, values cannot be set outside of the setter. In Godot 3, they could be set anywhere in the downstream setter callstack. The code posted would likely have worked in Godot 3.

I don't know if the change is intentionally designed or if this is an unintented bug. If the former, it should be documented somewhere with clear examples.

The original code recurses until the callstack fills up and the engine is killed:

@export var default_color : Color = Color.WHITE:
	set(value): 
		set_default_color(value)

func set_default_color(value):
	default_color = value    ### <- Calls default_color set(value) again, and the variable never gets set
	update()

One cannot fix it with either or both of these if statements. default_color and new color are always different. default_color never gets set because its outside set().

@export var default_color : Color = Color.WHITE:
	set(value):
		if default_color != value:
			print("Setting default color: old %s, new %s" % [default_color, value])
			set_default_color(value)

func set_default_color(value):
	if default_color != value:
		default_color = value    ### <- Calls default_color set(value) again

You can do this:

@export var default_color : Color = Color.WHITE:
	set(value):
		default_color = value
		update()

However if you only want to update when the value has changed, GD3 was more supportive of that as you could change values anywhere in the setter callstack. Now you have to do something like this:

@export var default_color : Color = Color.WHITE:
	set(value):
		var old_color: Color = default_color
		default_color = value
		if old_color != default_color:
			update()

@TokisanGames
Copy link
Contributor

TokisanGames commented Jul 3, 2022

Here's another example where this new behavior may be a problem for a lot of projects:

I'm calculating camera fov and focal_length. The values inversely correlate. Basically, both variables need to change simultaneously to be correct.

In Godot 3 I could set fov and have it change focal_length, or vice versa. Perhaps there was just a recursion checker that broke out of crappy code and now that's gone, idk. However right now I need to do additional work to update both values w/o triggering infinite recursion. The simple code below should not crash the engine. It should just print an error and stop the script.

This breaks the engine with infinite recursion when you change a value in the inspector (or upon scene load w/ nondefault values):

@tool
extends Node

@export var first: float = 1.0:
	set(value):
		first = value
		second = 2*first

@export var second: float = 2.0:
	set(value):
		second = value
		first = 0.5*value

Meanwhile, in Godot 3, this works fine w/o infinite recursion:

tool
extends Node

export var first: float = 1.0 setget set_first
export var second: float = 2.0 setget set_second
					
func set_first(value):
	print("Setting first: ", first)
	first = value
	second = 2*first
	property_list_changed_notify()

func set_second(value):
	second = value
	first = 0.5*value
	property_list_changed_notify()

This does work in GD4.

@export var first: float = 1.0:
	set(value):
		if first != value:
			first = value
			second = 2*first

@export var second: float = 2.0:
	set(value):
		if second != value:
			second = value
			first = 0.5*value

Then I have even another example where in a public function I set a variable, but the variable setter also calls this function. Since outside functions can't set variables w/ setters I'm still chewing on what sort of hackish solution will work around this.

e.g. here's a simplified example

@export focus_point: Vector3:
  set(value):
      focus_point = value
      focus_at(focus_point)

func focus_at(p_fpoint: Vector3.....):
      if some conditions:
             focus_point = p_fpoint
      .... focus stuff ...

In Godot 3 I can either set the focus_point in the inspector or via script, or I can call the focus_at function to have it recalculate. In GD4, I'm looking at using a hacky sentinel public variable or timer to avoid redundant calls to focus_at().


So before I spend time refactoring all of my setters throughout my game, I'd really like to know if this new behavior is just a bug that will be reverted back to GD3 behavior, or is it a new design philosophy?
That design being:

  1. Variables can only be set within their setter function, not a subroutine called by the setter.
  2. Things that probably should have caused infinite recursion like my gd3 example did not, but now they do.

@TokisanGames
Copy link
Contributor

So before I spend time refactoring all of my setters throughout my game, I'd really like to know if this new behavior is just a bug that will be reverted back to GD3 behavior, or is it a new design philosophy?
That design being:

  1. Variables can only be set within their setter function, not a subroutine called by the setter.
  2. Things that probably should have caused infinite recursion like my gd3 example did not, but now they do.

Hey @vnen, can you answer this question please? Thanks.

@Kwu564
Copy link

Kwu564 commented Aug 3, 2022

I was facing the same problem until I discovered an alternative syntax for the set function in Godot 4 which behaves differently from the those shown above.

Here is color modification example but redone with the alternative set syntax:

@tool
extends Node3D

@export var default_color : Color = Color.WHITE: set = set_default_color
		
func set_default_color(value):
	default_color = value

And here is the same color example but with each color variable setting each other:

@tool
extends Node3D

@export var default_color : Color = Color.WHITE: set = set_default_color
@export var default_color2 : Color = Color.WHITE: set = set_default_color2
		
func set_default_color(value):
	print("set default color")
	default_color = value
	if default_color2 == value:
		return
	default_color2 = value
	notify_property_list_changed()

func set_default_color2(value):
	print("set default color 2")
	default_color2 = value
	if default_color == value:
		return
	default_color = value
	notify_property_list_changed()

I tested each of these on my end and it works fine with no crashes at all. So maybe the change in the set function was intentional after all?

@Kwu564
Copy link

Kwu564 commented Aug 3, 2022

This was tested on the alpha 13 dev snapshot if that helps.

@TokisanGames
Copy link
Contributor

@Kwu564 Thank you for this.

Yes indeed, using the alternative format exhibits the Godot 3 behavior, allowing us to set variables in external setter functions, without any checks for recursion or special workarounds. I can confirm everything you wrote. The code that crashes the engine in my post above works as it did in GD3 if using your alternate format.

It's good that we now have a way to use the familiar behavior. But now in the engine has a lurking, engine crashing bug. Use the wrong format and descend into infinite recursion until your call stack blows up. Yikes. And even if the crashing is fixed, it's not good that setters have two entirely different behaviors depending only on code formatting!

I think a good solution would be to just remove the first format and behavior entirely. I don't like my setter function implementation in the middle of my variable declarations. And I certainly don't like the more strict behavior or the crashes. I've already been moving my code to the alt format to use the GD3 behavior.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 4, 2022

Everything is documented here: https://docs.godotengine.org/en/latest/tutorials/scripting/gdscript/gdscript_basics.html#properties

This is a standard example of infinitely recursive function. You can achieve the same with

func infinity():
	infinity()

There is no way to prevent such crashes other than limiting stack size (which I think no language does, given that stack overflow is a common error). The behavior of setters is not going to be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants