Fix Skeleton2D.{_set|_get} always returning true

[kleonc]

Fixes #95117.

Follow-up to #84060.
(@AThousandShips this was simply overlooked? 🤔)

(not sure what topic this should be labelled as)

[AThousandShips]

Mentioned them in #84054 but left them because I wasn't sure of the exact correct cases and didn't get any feedback on either PR for the correct approach so didn't touch them

[AThousandShips]

While we're here I feel the path.begins_with is suspicious, it should probably just match against the exact string and might be useful to just use SNAME if it's common enough

[KoBeWi]

Yeah, looking at the _get_property_list(), the modification_stack should use exact match with SNAME.

Skeleton 3D has a similar wrong return (it actually works correctly, but the code is awkward).

[kleonc]

There's the same problem in Bone2D (same file/author):

godot/scene/2d/skeleton_2d.cpp

Lines 41 to 99 in 3978628

	bool Bone2D::_set(const StringName &p_path, const Variant &p_value) {
	String path = p_path;
	if (path.begins_with("auto_calculate_length_and_angle")) {
	set_autocalculate_length_and_angle(p_value);
	} else if (path.begins_with("length")) {
	set_length(p_value);
	} else if (path.begins_with("bone_angle")) {
	set_bone_angle(Math::deg_to_rad(real_t(p_value)));
	} else if (path.begins_with("default_length")) {
	set_length(p_value);
	}
	#ifdef TOOLS_ENABLED
	else if (path.begins_with("editor_settings/show_bone_gizmo")) {
	_editor_set_show_bone_gizmo(p_value);
	}
	#endif // TOOLS_ENABLED
	else {
	return false;
	}
	return true;
	}
	bool Bone2D::_get(const StringName &p_path, Variant &r_ret) const {
	String path = p_path;
	if (path.begins_with("auto_calculate_length_and_angle")) {
	r_ret = get_autocalculate_length_and_angle();
	} else if (path.begins_with("length")) {
	r_ret = get_length();
	} else if (path.begins_with("bone_angle")) {
	r_ret = Math::rad_to_deg(get_bone_angle());
	} else if (path.begins_with("default_length")) {
	r_ret = get_length();
	}
	#ifdef TOOLS_ENABLED
	else if (path.begins_with("editor_settings/show_bone_gizmo")) {
	r_ret = _editor_get_show_bone_gizmo();
	}
	#endif // TOOLS_ENABLED
	else {
	return false;
	}
	return true;
	}
	void Bone2D::_get_property_list(List<PropertyInfo> *p_list) const {
	p_list->push_back(PropertyInfo(Variant::BOOL, PNAME("auto_calculate_length_and_angle"), PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT));
	if (!autocalculate_length_and_angle) {
	p_list->push_back(PropertyInfo(Variant::FLOAT, PNAME("length"), PROPERTY_HINT_RANGE, "1, 1024, 1", PROPERTY_USAGE_DEFAULT));
	p_list->push_back(PropertyInfo(Variant::FLOAT, PNAME("bone_angle"), PROPERTY_HINT_RANGE, "-360, 360, 0.01", PROPERTY_USAGE_DEFAULT));
	}
	#ifdef TOOLS_ENABLED
	p_list->push_back(PropertyInfo(Variant::BOOL, PNAME("editor_settings/show_bone_gizmo"), PROPERTY_HINT_NONE, "", PROPERTY_USAGE_DEFAULT));
	#endif // TOOLS_ENABLED
	}

So for exact properties it can be changed to p_path == SNAME("prop_name"). What about editor settings check like path.begins_with("editor_settings/show_bone_gizmo")? 🤔

Skeleton 3D has a similar wrong return (it actually works correctly, but the code is awkward).

I don't see a "wrong return" there. Unless you mean that it doesn't end with return false. It's indeed structured differently but overall looks fine to me.
The checks for animate_physical_bones look questionable though, also in case path.begins_with("animate_physical_bones") returns true then _get/_set would still return false.

godot/scene/3d/skeleton_3d.cpp

Lines 71 to 82 in 3978628

	bool Skeleton3D::_set(const StringName &p_path, const Variant &p_value) {
	String path = p_path;
	#ifndef DISABLE_DEPRECATED
	if (path.begins_with("animate_physical_bones")) {
	set_animate_physical_bones(p_value);
	}
	#endif
	if (!path.begins_with("bones/")) {
	return false;
	}

godot/scene/3d/skeleton_3d.cpp

Lines 141 to 152 in 3978628

	bool Skeleton3D::_get(const StringName &p_path, Variant &r_ret) const {
	String path = p_path;
	#ifndef DISABLE_DEPRECATED
	if (path.begins_with("animate_physical_bones")) {
	r_ret = get_animate_physical_bones();
	}
	#endif
	if (!path.begins_with("bones/")) {
	return false;
	}

[KoBeWi]

What about editor settings check like path.begins_with("editor_settings/show_bone_gizmo")?

If they check full name, begins_with() make no sense. The _set()/_get() argument is StringName, so using SNAME avoids conversion. It's fine without SNAME too though, for things that aren't accessed often.

I don't see a "wrong return" there. Unless you mean that it doesn't end with return false.

Yes, it's an unreachable code too. The else should be replaced with proper return.

[kleonc]

Yes, it's an unreachable code too. The else should be replaced with proper return.

I don't see any unreachable code in there so still not sure what you mean. 🙃

Anyway, I've pushed mentioned changes.

[AThousandShips]

so the return below is unreachable.

Not unreachable, it the other if statements don't return so they join in that:

 else if (what == "scale") {
	set_bone_pose_scale(which, p_value);

[akien-mga]

Thanks!

[akien-mga]

Cherry-picked for 4.3.1.