gilzoide / godot-lua-pluginscript

Godot PluginScript for the Lua language, currently based on LuaJIT's FFI
https://gilzoide.github.io/godot-lua-pluginscript/topics/README.md.html
MIT License
308 stars 21 forks source link

NodePath property is inaccessible in Lua #1

Closed JohnDoneth closed 3 years ago

JohnDoneth commented 3 years ago

Plugin Version: r1 Godot Version: v3.3.3

I'm trying to use a NodePath property and access it from Lua.

The code appears to register the property with Godot as it shows up and works in the inspector but is inaccessible via self in Lua. image

Properties are working for other primitive types I've used so far but not when the type is NodePath. When I inspect self inside of _ready with the following code, I get the subsequent outcome.

local inspect = require("inspect")

local function ready(self)
  print(inspect.inspect(self))
  print(self.player_path)
end

return {
    _ready = ready,
    player_path = property({type = NodePath})
}
{
  __owner = [Node2D:1622],
  __script = {
    __getter = {},
    __path = "res://enemy.lua",
    __setter = {},
    _ready = <function 1>
  },
  <metatable> = {
    __concat = <function 2>,
    __index = <function 3>,
    __tostring = <function 4>
  }
}
nil

As you can see, the property is not on self and printing the field directly shows that it doesn't exist, as it's nil.


I've been loving this project by the way. It's great! Thank you for working on it.

JohnDoneth commented 3 years ago

More Digging

Digging some more, I found that the property is in the property list that is returned by self:get_property_list().

print(self:get_property_list()[#self:get_property_list() - 1])

--> {class_name:, hint:0, hint_string:, name:player_path, type:15, usage:7}

Attempting to use self:get("player_path") directly instead of through the __index metamethod doesn't appear to work either, as it also returns nil.


Discovering a Solution

While looking through the code dealing with properties I noticed some mentions of the default value being kept for indexing purposes. The way I had the code did not have a default value. This lead to me wonder what would happen if I assigned the property player_path a new instance of NodePath directly instead of calling property with just a type specified. Doing so fixed my issue.

Code

local function ready(self)
    print(self.player_path)
    print(self:get_node(self:get("player_path")))
end

return {
    _extends = "Node2D",
    _ready = ready,
    player_path = NodePath()
}

Output

../Player
[Node2D:1604]

Conclusion

Maybe we should set default_value to a new instance of type, or always require a default value when calling property to avoid someone else getting stuck like me? I'll keep this issue open if you feel that's actionable, or we can create a new more focused issue. Feel free to close this otherwise.

Thanks again for the great project!

gilzoide commented 3 years ago

Hi there! I see you already found out how properties are working currently. Yeah, the property is registered in ClassDB, but without an initial value, getting it would return null on GDScript. But you're right, there is a bug when the default value is nil, in a way that the "get property" callback thinks that the property doesn't exist!

I think the way to go is really adding a fallback default value for properties, when absent, thanks for pointing it out! Requiring a default value is reasonable too, but having fallback empty/zero/identity values seems nicer to me. Tracking known properties in a separate __properties table is probably a good idea too.

Also, now that I'm thinking better about this, just falling back to __indexing the script will be very error-prone for Arrays, Pool Arrays, Dictionaries and Object values =X When instancing the script, we can iterate over known properties and call a constructor function for each one. I'll make this happen =]

Either way, we can always stuff initializers in the _init method, although it is much nicer to just define initial values to be created.


Not related but more out of curiosity, what OS are you using? Also, did you use the prebuilt release .zip file? Thanks again for the help, I'm really glad you like it ^^

gilzoide commented 3 years ago

Just FYI, now that I'm remembering more stuff, the idea was that default value was sort of required, but I though that having null as default was not necessarily bad, as it might just be what people want, so just let it roll. Now I see that a default default value is better and is probably what most people would want, and getter methods may be used for defaulting to null.

But the "get property" bug still applies, so in the end it was a really bad choice =P

JohnDoneth commented 3 years ago

Thank you for the detailed explanation. I like your solution and I'm glad I could help.

Not related but more out of curiosity, what OS are you using? Also, did you use the prebuilt release .zip file?

I'm using Arch Linux and I did use the r1 release zip. It was plug-and-play. Very easy to get started.

gilzoide commented 3 years ago

I'm using Arch Linux and I did use the r1 release zip.

I asked because I use mainly a Manjaro Linux, so was curious about how this plugin would behave on other systems. I did test on Wine and on an OSX a couple of times, though, and it also worked on my attempts. But who knows, right?

It was plug-and-play. Very easy to get started.

Awesome!