Bug Report: Vector3 metatable not working when nested in an object

[jbromberg]

Describe the bug
When pushing an object variant that contains a Vector3 property, it's not updated correctly. Only global level Vector3s are being updated.

To Reproduce

class LuaClass:
	var vec := Vector3.ZERO

var vec := Vector3.ZERO

var lua := LuaAPI.new()
lua.push_variant("vec", vec)
lua.push_variant("luaClass", LuaClass.new())
lua.do_string("
vec.x = 10
print(vec) -- (10, 0, 0)
luaClass.vec.x = 10
print(luaClass.vec) -- (0, 0, 0)
")

[jbromberg]

Seems to not work on other nested types such as godot arrays too.

[jbromberg]

Works with nested basic types like bool, float, and string

[Trey2k]

This is a side effect to how we handle metatables. Properties are not passed by reference unless they are a Object type. So types like Vector3 what ends up happening is obj.vec pushes a copy of the vector onto the stack. obj.vec.x = 10 modifies the copy of the vector and not the objects vector. And since the first obj.vec is only a index call and not a newindex call the copy is not reapplied to the object ever.

Realevent areas of the code base are the following.

godot_luaAPI/src/metatables.cpp

Lines 176 to 192 in edf4afe

	LUA_METAMETHOD_TEMPLATE(L, -1, "__index", {
	if (arg1.has_method(arg2.operator String())) {
	lua_pushlightuserdata(inner_state, lua_touserdata(inner_state, 1));
	LuaState::pushVariant(inner_state, arg2);
	lua_pushcclosure(inner_state, luaUserdataFuncCall, 2);
	return 1;
	}
	LuaState::pushVariant(inner_state, arg1.get(arg2));
	return 1;
	});
	LUA_METAMETHOD_TEMPLATE(L, -1, "__newindex", {
	// We can't use arg1 here because we need to reference the userdata
	((Variant *)lua_touserdata(inner_state, 1))->set(arg2, arg3);
	return 0;
	});

godot_luaAPI/src/classes/luaObjectMetatable.cpp

Lines 197 to 213 in edf4afe

	Variant LuaDefaultObjectMetatable::__index(Object *obj, Ref<LuaAPI> api, Variant index) {
	if (obj->has_method("__index")) {
	return obj->call("__index", api, index);
	}
	Array fields = Array();
	if (obj->has_method("lua_fields")) {
	fields = obj->call("lua_fields");
	}
	if ((!permissive && fields.has((String)index)) || (permissive && !fields.has((String)index))) {
	return obj->get((String)index);
	}
	return Variant();
	}

godot_luaAPI/src/classes/luaObjectMetatable.cpp

Lines 214 to 237 in edf4afe

	Ref<LuaError> LuaDefaultObjectMetatable::__newindex(Object *obj, Ref<LuaAPI> api, Variant index, Variant value) {
	if (obj->has_method("__newindex")) {
	Variant ret = obj->call("__newindex", api, index, value);
	if (ret.get_type() == Variant::OBJECT) {
	#ifndef LAPI_GDEXTENSION
	return Object::cast_to<LuaError>(ret.operator Object *());
	#else
	return dynamic_cast<LuaError *>(ret.operator Object *());
	#endif
	}
	}
	Array fields = Array();
	if (obj->has_method("lua_fields")) {
	fields = obj->call("lua_fields");
	}
	if ((!permissive && fields.has(index)) || (permissive && !fields.has(index))) {
	obj->set(index, value);
	return nullptr;
	}
	return LuaError::newError(vformat("Attempt to set field '%s' on object of type '%s' which is not a valid field.", index, obj->get_class()), LuaError::ERR_RUNTIME);
	}

I will need to spend some time thinking on this to find a good solution. This is not the intended behavior.

[jbromberg]

Sounds good. Would it be possible to somehow recursively instantiate object properties the same way globals are created? I'm pretty new to Lua so not sure if it works like this but basically instead of just copying all the object's properties we could iterate over them and if they're a non basic type they get their own metatable like the globals do. If it encounters another object, it recurses and thus it could work for several layers of nesting & complex data types.

[Trey2k]

Sounds good. Would it be possible to somehow recursively instantiate object properties the same way globals are created? I'm pretty new to Lua so not sure if it works like this but basically instead of just copying all the object's properties we could iterate over them and if they're a non basic type they get their own metatable like the globals do. If it encounters another object, it recurses and thus it could work for several layers of nesting & complex data types.

So this is not actually the issue here. The data type has a metatable. The issue is the vector being modified is not the objects vector but a copy. If a property on the object is modified IE the call goes through __newindex we update it via reference like with obj.prop=val. But if its not modified directly on the object IE the call goes through __index like with obj.prob.x=val the property is pushed as a copy. Then that copy is what gets modified.

So in your example if you changed the lua code to

vec.x = 10
print(vec) -- (10, 0, 0)
luaClass.vec = Vector3(10, 0, 0)
print(luaClass.vec) -- (10, 0, 0)

It would then work as expected.

[Trey2k]

A fix would be to always push properties by ref. But I think that would create other issues. some_vec = obj.vec you wouldnt expect some_vec to be a ref to obj.vec in that case but it would end up being so.

[jbromberg]

Makes sense. So for globals it just modifies the vector directly on the global table instead of being a copy, but it's a copy for nested types?

Another idea could be to somehow pass __newindex calls up the owner chain. So if a child vector gets a new property assigned and it's being referenced from an object the object would get the new vector which got the new property.

Or could push by ref but create copies on assignment, so some_vec = obj.vec would copy

[MasterDingo]

The last option seems to be the most reasonable one.