SpineSkin.new() does not instantiate spine::Skin properly

[scottkunkel]

SpineSkin.new should require a string as an argument

get_skin_name should work right after instantiating a new skin in godot (currently crashes)
add_skin and copy_skin should work when adding existing skins to a newly instantiated skin

CustomSkinProject.zip

[jonchun]

I'm not a C++ guy so this solution is most likely wrong. However, I did a little bit of research:

godotengine/godot#23260
https://godotengine.org/qa/133/how-to-add-a-gdscript-constructor-to-a-c-class

and it looks like GDNative/C++ godot modules don't support arguments in new()?

My solution was to add a new method called setup_spine_skin() which could be used to instantiate a new spine skin.
jonchun@839fd7a

I'm able to get custom skins working as follows:

	var custom_skin: SpineSkin = SpineSkin.new()
	custom_skin.setup_spine_skin("custom_skin")
	var hair = spine_sprite.get_skeleton().get_data().find_skin("hair/brown")
	custom_skin.add_skin(hair)
	spine_sprite.get_skeleton().set_skin(custom_skin)

I realize that this is a non-standard API and might cause issues down the line, so hopefully someone else has a better solution.

[jonchun]

My other thought was to perhaps just add a get_new_skin(name) method to SpineSprite which returns an empty SpineSkin with name correctly set.

Usage would then be

onready var spine_sprite: SpineSprite = $SpineSprite
func _ready() -> void:
	var custom_skin: SpineSkin = spine_sprite.get_new_skin("custom_skin_name")

I'm leaning towards this solution for now, but will wait to see if anyone else has more input.

[scottkunkel]

I like attaching it to spinespire to keep the API clean.
Alternatively, I did this:

SpineSkin::SpineSkin() {
	const String &name = "custom";
	skin = new spine::Skin(S_T(name));
}
SpineSkin::~SpineSkin() {
	delete skin;
}

requiring no changes to the api. Not sure about hardcoding the name but you'd call up a new skin every time you recustomize the sprite.

Cleaner would be to assign a uuid to the name

[jonchun]

requiring no changes to the api.

Adding it to the constructor was my initial thought as well, but I didn't want to think through the implications of whether I would ever need a custom name. However, upon thinking about it, I think I'll combine what you did (with a hardcoded name of "custom") and then additionally add the get_new_skin() to SpineSprite as a non-standard call to just solve both problems.

---

I tried doing it in the constructor exactly the way you have it and my Godot starts freezing/not playing nicely. Did you make any additional changes to make that work?

[rayxuln]

I've come up with a solution:

func test3() -> void:
	# expect to see brown hair. see brown hair
	var custom_skin: SpineSkin = SpineSkin.new().init("my_custom_hair")
	var hair = spine_sprite.get_skeleton().get_data().find_skin("hair/brown")
	custom_skin.add_skin(hair)
	spine_sprite.get_skeleton().set_skin(custom_skin)
	spine_sprite.get_skeleton().set_to_setup_pose()

I added a init function to SpineSkin that requires a name and return a fully functional SpineSkin instance since godot does not support arguments in new function of GDNativeScriptClass.

I've also added this example to the Spine Runtime For Godot Example Project, and the tests are all working as expected.

[jonchun]

Thanks @rayxuln. Works great. I guess there's really no difference between what you implemented (just named init instead of setup_spine_skin) from my initial solution.

Do you prefer this non-standard API approach vs having a factory function in SpineSprite?

[rayxuln]

I would like to have a static factory function in SpineSkin class but can't find a way to do that. The init function is working the same as the new function and I think it's cleaner then adding a new function in SpineSprite class.

[scottkunkel]

I'm ok with the init function. We have to work with the limitations of godot (non parameter instantiation) so it's already breaking the standard api.

We can add this to the doc classes. That way it's documented going forward.

[jonchun]

Agreed. I think we can close this out then?