Fix invalid return from some more _get/_set

[AThousandShips]

Invalidly returned true on the non-matched path

Follow-up to:

• Fix invalid return from some _get/_set #84054

Reviewers: If these cases are invalid (i.e. all paths should return true) then please explain why and I'll add a note for it instead

[AThousandShips]

This is enforced by format, unless I move the closing brackets into the #ifdef

[KoBeWi]

Given that all classes with the wrong code are related, the person who implemented these methods probably didn't know how to use them correctly. There is no reason to write it like that.

You should return false at the end of each method instead of adding else. true should be returned only when r_ret is modified.

[AThousandShips]

You should return false at the end of each method instead of adding else.

Not unless you also add return true in all the if blocks, that's why the trailing true is there, this syntax is used in a lot of valid places as well, just not this way

See for example Animation::_set/_get

[KoBeWi]

Not unless you also add return true in all the if blocks

Yep, the blocks should return.

See for example Animation::_set/_get

That one is wrong too.

[AThousandShips]

I mean this syntax is used in plenty of places, can modify it here to simplify it but kept the changes minimal

[AThousandShips]

Compare:

bool SkeletonModification2DLookAt::_get(const StringName &p_path, Variant &r_ret) const {
	String path = p_path;

	if (path.begins_with("enable_constraint")) {
		r_ret = get_enable_constraint();
	} else if (path.begins_with("constraint_angle_min")) {
		r_ret = Math::rad_to_deg(get_constraint_angle_min());
	} else if (path.begins_with("constraint_angle_max")) {
		r_ret = Math::rad_to_deg(get_constraint_angle_max());
	} else if (path.begins_with("constraint_angle_invert")) {
		r_ret = get_constraint_angle_invert();
	} else if (path.begins_with("constraint_in_localspace")) {
		r_ret = get_constraint_in_localspace();
	} else if (path.begins_with("additional_rotation")) {
		r_ret = Math::rad_to_deg(get_additional_rotation());
	}
#ifdef TOOLS_ENABLED
	else if (path.begins_with("editor/draw_gizmo")) {
		r_ret = get_editor_draw_gizmo();
	}
#endif // TOOLS_ENABLED
	else {
		return false;
	}

	return true;
}

To:

bool SkeletonModification2DLookAt::_get(const StringName &p_path, Variant &r_ret) const {
	String path = p_path;

	if (path.begins_with("enable_constraint")) {
		r_ret = get_enable_constraint();
		return true;
	} else if (path.begins_with("constraint_angle_min")) {
		r_ret = Math::rad_to_deg(get_constraint_angle_min());
		return true;
	} else if (path.begins_with("constraint_angle_max")) {
		r_ret = Math::rad_to_deg(get_constraint_angle_max());
		return true;
	} else if (path.begins_with("constraint_angle_invert")) {
		r_ret = get_constraint_angle_invert();
		return true;
	} else if (path.begins_with("constraint_in_localspace")) {
		r_ret = get_constraint_in_localspace();
		return true;
	} else if (path.begins_with("additional_rotation")) {
		r_ret = Math::rad_to_deg(get_additional_rotation());
		return true;
	}

#ifdef TOOLS_ENABLED
	if (path.begins_with("editor/draw_gizmo")) {
		r_ret = get_editor_draw_gizmo();
		return true;
	}
#endif // TOOLS_ENABLED

	return false;
}

Can go with the second syntax if desired, but I think it's for a separate PR cleaning up and unifying the style

[YuriSizov]

I think the existing syntax has the right to be if there are further actions required after the if-else block. If not, returning early should be clearer, since it indicates that this is the last step and you don't need to read any further. But I also agree it can be pretty noisy.

So no strong opinion either way.

[KoBeWi]

Ok I misunderstood the code. This syntax is indeed better.

[fire]

The convention is that we have many set and gets, there's a complicated switch / if else statement that else returns the the least likely and then falls through to the most likely Boolean value.

[akien-mga]

Thanks!

[AThousandShips]

Thank you!

[YuriSizov]

Cherry-picked for 4.1.4.