godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.6k stars 21.28k forks source link

Inconsistent behaviour of setup methods between GDScript and GDNative (C++) - values and default values of properties #26237

Open mnn opened 5 years ago

mnn commented 5 years ago

Godot version: v3.1.beta6.official 64b

OS/device including version: Linux 64b


Issue description:

By changed value and default value I mean values in text fields in Script Variables in the editor.

GDNative: In _init property values are not initialized at all. In _ready properties which should have default values are not initialized to default values, properties with changed values are properly handled.

GDScript: In _init property values are initialized (to default values). In _ready changed properties are initialized to their current (changed) values.

I hit this issue many times even in a trivial benchmark project and was for some time really confused, why setting properties in the editor works only in some cases.

In my opinion both of these should behave exactly same. I am guessing the GDScript implementation is the desired one, since it makes more sense.


Steps to reproduce:

...
  register_property<GDExample, float>("defaultValue", &GDExample::defaultValue, 5);
  register_property<GDExample, float>("changedValue", &GDExample::changedValue, 10);
...
void GDExample::_init() {
  Godot::print("[C++] init, defaultValue = {0}", defaultValue);
  Godot::print("[C++] init, changedValue = {0}", changedValue);
}
...
void GDExample::_ready() {
  Godot::print("[C++] ready, defaultValue = {0}", defaultValue);
  Godot::print("[C++] ready, changedValue = {0}", changedValue);
}
extends Node

export var defaultValue = 5;
export var changedValue = 10;

func _init():
  print("[GDScript] init, defaultValue = ", defaultValue)
  print("[GDScript] init, changedValue = ", changedValue)

func _ready():
  print("[GDScript] ready, defaultValue = ", defaultValue)
  print("[GDScript] ready, changedValue = ", changedValue)

Output:

[C++] init, defaultValue = 0
[C++] init, changedValue = 0
[GDScript] init, defaultValue = 5
[GDScript] init, changedValue = 10
[C++] ready, defaultValue = 0
[C++] ready, changedValue = 11
[GDScript] ready, defaultValue = 5
[GDScript] ready, changedValue = 11
akien-mga commented 5 years ago

CC @karroffel @BastiaanOlij

BastiaanOlij commented 5 years ago

Hmm, not sure, I've never dived into this part before.

In theory the declarations should result in the setters for these properties being called. It should work the same as binding the script to a node and then setting values on that node, they should default to these values, but then the setter should be called with whatever value is specified.

When registering properties with a pointer to the variable it should just use default getters and setters. It's worth testing if the same behavior exists with properties that have their own getter and setter method, it might just be a problem with the default setter.

I'm not sure when I'll have some time to see if I can find the issue, @karroffel you have any ideas?

karroffel commented 5 years ago

That is because the _init function is just convention and not enforced by anything inside of Godot. GDScript has its own way to calling it, C# has its own, VisualScript has it own. It's not a property of the Godot object system that the _init function is called, just as a bit of context.

NativeScript does not call a _init function at all! Instead, this is left to the bindings.

The C++ bindings register a function that Godot calls. This function constructs the actual user object. After calling the constructor it also calls the _init function, this happens before control is given back to Godot.

https://github.com/GodotNativeTools/godot-cpp/blob/05e5f5cd5e88e5d7ed408a421424443c1571eeb3/include/core/Godot.hpp#L98-L105

So the _init function in the C++ bindings is just there for the convention of "the _init function can be used to initialize an object", because when the constructor is called the "owner" of the new object is not set, which means no Godot functions can be called. In the _init the owner is set, so all available methods can be called.

I hope this clarifies things, I'm not sure if the behavior should be changed and the _init function should be called from the NativeScript implementation or if it's not worth it...

mnn commented 5 years ago

What about ready? I don't see how it could be a correct behaviour to not set default value in C++:

[C++] ready, defaultValue = 0
[GDScript] ready, defaultValue = 5

Edit: I think wrong default value in ready on onward is much bigger issue than inconsistent init.

karroffel commented 5 years ago

I'm not sure why they are not set in _ready, but there is also a functional difference in the source code you sent.

When you write export var defaultValue = 5 in GDScript, that exposes 5 as the default value for that property and initializes the value to 5 in the constructor.

In the C++ code you do not set the values inside the constructor. I assume that the setter is never called in GDScript either, but you don't see it since the value is the same.

mnn commented 5 years ago

Why isn't used "default value" from registering property? From where I sit, 5 is the default value and should work in a same way as export var defaultValue = 5 in GDScript does:

  register_property<GDExample, float>("defaultValue", &GDExample::defaultValue, 5);

Since the error of forgetting "default values" are actually not default values, so they have to be set manually, is so common, I stopped using "default values" as default values. I always pick a value which does not make any sense and crashes game instantly, forcing me to set it in the inspector. All to avoid (IMO bug) where default value from registering property is not used and not set during initialization. This is very counter intuitive and not a functionlity I would ever call a "default value"...

karroffel commented 5 years ago

I would say this is a problem with C++ first, then it's an issue with Godot not actually setting the default value.

I can see why Godot does that. It needs the information of the default value to provide the "revert" button in the inspector. It probably does not actually set the value (why would you, it's the default value after all..).

So I would say it just needs to be made explicit that members have to be initialized with the default value explicitly, since Godot does not do it.

mnn commented 5 years ago

Okay, I am confused. I am a mere user of Godot and the tools around (C++ bindings, GDNative, C headers; I though C++ support is official?). I am not sure what you mean by

...this is a problem with C++ first... So I would say it just needs to be made explicit that members have to be initialized with the default value explicitly, since Godot does not do it.

Do you mean that a user of an engine should be forced to specify default value twice? If so, why? GDScript doesn't have such IMO bad approach. Why would anybody want to specify two default values, each with a different value? What would be a use of it?

karroffel commented 5 years ago

So, it's not like the engine expects you to do it twice. Giving the default value is just a hint to Godot. When a user changes the value then the editor can reset it to the "default value" if desired. The default value should reflect the initial value of the property.

And this is where C++ vs GDScript comes in.

In GDScript, if you say export var x = 42 then Godot creates an implicit constructor for this object. It will set the initial value of x to 42 and it will also register a property, which has a default value of 42.

In C++, if you say register_property("x", &MyClass::x, 5.0f) then you tell Godot "hey, here is a property, you can find it like that, it has this name, and when somebody wants to reset it please use this value". BUT if you leave the constructor of the class empty, the actual initial value will be undefined!

It's not like Godot forces you to do this twice, it's Godot expecting the language you are using to initialize the object properly and give some information, if that is needed.

C++ is more low level than GDScript. GDScript does this for you, in C++ you have to explicitly set the initial value in the constructor.

mnn commented 5 years ago

My question still stands, why? If there is no reason to have two different default values (honestly, I don't think it should be even allowed), why force C++ users to write boilerplate?

Again, maybe this should be on C++ bindings "issues" page, I don't know, I am simply voicing my experience with using C++ with Godot and how inconsistent and border-line broken it is when it comes to default values. If that's the case, please say so and I'll re-post it to appropriate project.

karroffel commented 5 years ago

No, there is no reason to have different default and initial value. But you just can't express that in C++, which is why you have to do it manually.

In GDScript you can express that, the export var = ... does exactly that, but there is no good way in C++ to create both the register_property call with the default value and the initialization in the constructor using only one language construct.

If you have an idea how to do that, I'd gladly accept a PR in the C++ bindings, but I could not come up with a way to do it, so it is separate for now.

aaronfranke commented 4 years ago

Per @karroffel's comments, it doesn't seem like there is any actionable bug, but this needs to be documented.