Setter not called in self

[bojjenclon]

Me again! In GDScript if you have a setter on a property and change it via self.property the setter will be used. This doesn't apply to Lua, so I'm wondering how to trigger the setter from within a class method?

[gilzoide]

Hi there!
Right now, to trigger a setter from Lua, just call the setter method or set explicitly, like:

self:set_some_property(value)
-- or
self:set('property', value)

By the way, in GDScript, to call a setter method of a property defined in the script itself, you also have to explicitly call the method. This avoids recursion when the setter method sets the property itself and gives fine control over when calling the setter method is really needed.

var some_property setget set_some_property

func set_some_property(value):
  # this does not call set_property again recursively
  some_property = value

func some_unrelated_method():
  # in fact, anywhere in this script that sets this
  # property like this will not call the setter method
  some_property = 42

func _ready():
  # ok, now calling setter method
  set_some_property(42)

---

Now that I'm thinking of it, though, setting properties on self that are defined in base classes will not set them properly, which is bad =/

-- this will set the position in the script instance table, not the Node2D object
some_node2d.position = Vector2(1)
-- calling `set_position` or `set` is still the way, right now
some_node2d.set_position(Vector2(1))

A __newindex metamethod that tries to set the property first or check if the property exists for the object or class could work.

-- Version 1
-- Problem with this is that calling `set` would convert
-- `value` to a Variant, so big tables would be converted to
-- Dictionaries just to potentially be thrown away.
-- Also, right now functions and coroutines cannot be
-- transformed into Variants, although they could be, by
-- incapsulating them as Objects
function ScriptInstance:__newindex(key, value)
  if not self:set(key, value) then
    rawset(self, key, value)
  end
end

-- Version 2
function ScriptInstance:__newindex(key, value)
  -- `has_property` doesn't exist, but would potentially
  -- search results from `Object.get_property_list` or
  -- `ClassDB.class_get_property_list`.
  -- Generating an Array and making a linear search every
  -- time __newindex is called doesn't seem too good, but we
  -- could cache the property names in the Script, which
  -- `self` always maintains a reference to
  if self:has_property(key) then
    self:set(key, value)
  else
    rawset(self, key, value)
  end
end

I like version 2 better, with some caching of the property names either eagerly when loading the script or lazily when the first __newindex gets called.

[bojjenclon]

By the way, in GDScript, to call a setter method of a property defined in the script itself, you also have to explicitly call the method. This avoids recursion when the setter method sets the property itself and gives fine control over when calling the setter method is really needed.

Actually you can just use self.property_name to use the setter. If you look at the docs: https://docs.godotengine.org/en/stable/getting_started/scripting/gdscript/gdscript_basics.html
Look at the Setters/getter section, near the bottom:

"As said, local access will not trigger the setter and getter. Here is an illustration of this:"

func _init():
    # Does not trigger setter/getter.
    my_integer = 5
    print(my_integer)

    # Does trigger setter/getter.
    self.my_integer = 5
    print(self.my_integer)

Now that I'm thinking of it, though, setting properties on self that are defined in base classes will not set them properly, which is bad =/

I'd actually noticed that recently as well, just thought maybe I was doing something wrong and didn't want to spam you with issues. :P

I think your fix for it sounds like a good idea. I'd likely lean toward lazy caching to avoid a huge expense for deeply nested hierarchies, but that's just my initial thought.

[gilzoide]

Oooh, now I got it, thanks!
Guess I never payed attention to that part of the docs =X

I'd actually noticed that recently as well, just thought maybe I was doing something wrong and didn't want to spam you with issues. :P

Don't worry, that's what issues are for. This is the kind of thing that easily gets unnoticed, since for the time being I haven't done any real projects with Lua PluginScript.

[gilzoide]

As for __newindex, we have to think well how to approach it. The thing is it would be called only when current value for the property is nil. Properties registered in the script get initialized to their default values when a new instance is created, so a setter method would not be called for them unless we had the data stored in another table or something like that, which I think would be too much.

I think for self going explicit is fine and __newindex could only call set for properties defined in base classes. The setter methods are most likely available directly like self:set_property(value) and calling set explicitly is always an option.

The annoying thing is that self still wouldn't follow the behavior of other objects. So calling object.property = value on a Godot Object calls its set method, but self.property_defined_in_script = value does not for most of the time.

[bojjenclon]

Honestly I'm now wondering if perhaps it'd be best to encourage usage of set in Lua scripts. Given the nature of Lua tables as classes, trying to get proper setter behavior could potentially get complicated in __newindex.

[gilzoide]

Honestly I'm now wondering if perhaps it'd be best to encourage usage of set in Lua scripts.

Maybe, yes, I don't know for sure.
I'd still like to have support at least for properties from base classes in __newindex.
Either way, this issue about setting properties should will be properly documented at some point.

Given the nature of Lua tables as classes, trying to get proper setter behavior could potentially get complicated in __newindex.

If we were creating our own class model in pure Lua, I would agree.
But since we have Godot's class model, relying on Object.get_property_list or ClassDB.class_get_property_list for getting known properties is fine and should cover most cases.

[bojjenclon]

I think I'm going to trust your expertise on this one :) In my code base I've started opting to use set/get to help differentiate Lua table usage from Godot objects. But I can also totally understand wanting the interaction between the two sides to be cleaner than that. I'm down for whichever approach you choose.

[bojjenclon]

Oh btw, one other thing I noticed with setters that I'm not sure if its intended or not:

When defining a setter, I actually have to define it as set = function (self, key, value). Should that key part be required?

Ex:

local Object = {
  extends = "Node",

  prop = property({
    default = 0,
    set = function (self, key, value)
      -- Here "key" is just "prop" (the name of the property)
      self.prop = value
    end,
  }),
}

[gilzoide]

Should that key part be required?

No, that's totally a bug 😅 Only _set needs the property name.
At least this is an easy one, removing the name from the setter call will fix this.

[gilzoide]

I finally got back to messing with Lua PluginScript again.

I've discovered that aside from native Godot classes, we can only inherit PluginScripts, as far as 3.4 is concerned.
Also, for that to actually work, we'd need to handle inheriting properties and methods manually, so for now we can focus in native Godot classes only.

Thus I'm thinking that we can track available properties for native classes in ClassWrapper, reference this ClassWrapper in scripts metadata, then call set for them in __newindex. Seems simple and effective enough to make Lua scripts usage more enjoyable and far less error-prone =]

[bojjenclon]

Sounds promising, thanks for circling back around to this.

[gilzoide]

There it is, calling set for known properties from base classes.
In the end there was no need to store the ClassWrapper in scripts, just hitting the cached ones based on their names (self:get_class()) was enough.

[bojjenclon]

Legendary my man. If you want I can download the latest branch and try it out in my project. Unless you think you'll be pushing a new release soon.

[gilzoide]

I'll probably create a new release today, there are enough changes already and I usually only work on this on the weekends.
But feel free to download and try it out =]
I'll try making a CI pass to generate the whole distribution zip file for every commit in the main branch, this should make testing revisions easier.

[gilzoide]

@bojjenclon I was able to make a GitHub Action that not only builds all variations of the library, but creates the full distribution zip on push events.
And it took < 3 minutes, cool!
I really like this CI stuff xD

Here is a successful run of it, you should be able to download the lua_pluginscript-* zip and import in Godot (I still have to test this import workflow).
This run has the latest code, so set should be called for known properties of base classes.

[bojjenclon]

Sick, having CI on this will make testing updates even faster. I'm currently using set/get throughout most my code, so I'll need to make a few changes to test your work, but I'll give it a try so we can root out any potential issues.

[gilzoide]

Sick, having CI on this will make testing updates even faster

That's the idea ;)

I'll need to make a few changes to test your work

If it's too much trouble, don't worry. But if you do try, please tell if you find anything, whether it works or not.

[bojjenclon]

I can try to dig in a bit more later, but I did a really simple test where I simply took a property with a defined setter and changed the line from:

self:set("energy", self.energy - ceil((self.energy_per_hour * global_mod * mod) * dh))

to

self.energy = self.energy - ceil((self.energy_per_hour * global_mod * mod) * dh)

Unfortunately it doesn't seem to have worked though. :/ I don't see the value being changed properly in my GUI, whereas with set it is updated. I'd expect the behavior to be the same.

[gilzoide]

Is energy a property from a Godot class, such as Light2D or GIProbe?
If it is, does your script explicitly inherit from it or any subclass of it?

[bojjenclon]

Nope its a custom property:

energy = property({
  type = int,
  get = function (self) return self._energy end,
  set = function (self, value)
    self._energy = value

    if value <= 0 then
      self._energy = 0

      Coordinator:emit_signal("actor:energy_depleted", self)
    end

    if value > 100 then
      self._energy = 100
    end

    Coordinator:emit_signal("actor:energy_changed", self)
  end,
}),

[gilzoide]

Hmm, ok. Right now set on __newindex is being called only for properties in base classes, not the ones from your script.

The reasoning for that is mostly because of how __newindex works with tables, that they would not be called if self had any value with the key energy in this case, leading to inconsistent behavior.
Now, I know that the same applies for the properties of base classes, but script instances already initialize their own properties to default values, so that __newindex for them wouldn't be called most of the time, which won't happen for base class properties unless people redefine the property in script or go rawsetting values.

Now that I'm thinking of it, I guess getters have the same problem of not being called when the key exists in the table.

---

Ok, so while writing this, I was rethinking about script instances' design in Lua. Maybe they shouldn't be tables, but rather a struct which would have a consistent usage of __newindex. A Lua table would then be associated with it and used in both __index and __newindex, maybe available directly through GD.get_lua_instance or self.table or something like that if it seems fit.
Setter functions could be triggered by default, bypassed by a rawset method so that self:rawset('property', value) would just write the value to the table without triggering setters. Or use the idiom self.table.property = value, I don't know.

Lots of possibilities, but yeah, that having more consistent behavior regarding getters/setters would be better than how it is today.

[bojjenclon]

Ah I see, that was a misunderstanding on my part then. I'd definitely say it could be a nice enhancement to improve that workflow, if for nothing else than to ease transition between gdscript and lua. I'd personally say its not the highest of priorities though - if you have other areas to improve first - since set and get work just fine and are probably more "pure" as far as Lua is concerned anyway. Might just want to add a few more notes on how it behaves in the docs?

[gilzoide]

I'd definitely say it could be a nice enhancement to improve that workflow, if for nothing else than to ease transition between gdscript and lua.

I agree, I really want Lua PluginScript to be easy to use and transition from GDScript.
This kind of inconsistent behavior is very error-prone, even for pros at Lua and who know well how Lua PluginScript works.
Since script instances are so core to the usage of the plugin, I'm really bothered about this issue and want to have it fixed for the next release.

Might just want to add a few more notes on how it behaves in the docs?

Yes, this kind of documentation is a must, even though it isn't in place yet (hehe, my fault there).
I've been wanting to write a "Writing Lua scripts" and/or "From GDScript to Lua" docs for some time, this kind of behavior would fit both of them.

I'll keep you posted.

[gilzoide]

Ok, I've created a new branch to test this idea of having Script Instances be structs with a backing table for data.
So far it seems to work fine, I made a test that setter is being called for a property that is already there.

But the overall functionality needs better testing, to see if nothing broke and that indexing properties make sense in that they call the right getters/setters and that they index the owner Object.
I'll see if I make a simple game/app with this version some time this week, but feel free to test it out as well.

[bojjenclon]

Work has kept me insanely busy lately so I haven't had time to even crack open Godot in a while. :/ If I get a chance to test it at some point I definitely will, but your test suite may end up having to do for a while. Awesome that you're making progress on it though.

[gilzoide]

Work has kept me insanely busy lately

I totally get it, don't worry.

but your test suite may end up having to do for a while

It needs way more testing, but I'm sure we'll get to that at some point =]

I still haven't taken the time to make a game/app project with Lua to test the whole plugin experience better. I'm sure I'll get to that at some point as well =P
Until then, we keep this opened and keep in touch.

[gilzoide]

I've tested the solution some more and finally merged the PR.
I've also added entries in our "From GDScript to Lua" document about setter/getter functions, so I'm closing this issue.
(finally =P)