Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SkeletonProfile's is_require(), and set_require() aren't exposed #89892

Closed
catprisbrey opened this issue Mar 26, 2024 · 7 comments · Fixed by #89970
Closed

SkeletonProfile's is_require(), and set_require() aren't exposed #89892

catprisbrey opened this issue Mar 26, 2024 · 7 comments · Fixed by #89970

Comments

@catprisbrey
Copy link

catprisbrey commented Mar 26, 2024

Tested versions

4.2.1 Stable

System information

Ubuntu Linux, Mobile+

Issue description

While setting parameters for a Bone Map's SkeletonProfile in code, i found the set_require(), and is_require() don't seem to be available. The Docs don't list them as available , but the resource itself does seem to have it coded for use, (but i don't know C#/C++, so i could just be a dumb-dumb) but those fields are handy for setting values in code rather than manually in the inspector per each 50+ bones of a skeleton profile. I don't understand well enough why they're not usable:

void SkeletonProfile::set_require(int p_bone_idx, const bool p_require) {

What i'd like to see as far as expected behavior goes, is that the "require" checkbox for the bone in the inspector, be set-able in code. Either a set_require, or other method to directly set the value outside of the GUI. Or for somebody to explain how to do it since i could just be missing something.

Steps to reproduce

Create a script, export a skeleton profile, and then begin setting parameters for the bones inside it in code. You can set anything they ask for in the Inspector except for the 'Require" checkbox. In my case making a tool to do this for me, i used this code. My MRP will contain this in action and circumstance.

@tool
extends Skeleton3D

@export var refresh :bool = true : set = set_refresh # my cheap refresh button
@export var bone_template  = SkeletonProfile.new()

func _set_test(new):
	print()

func set_refresh(_new):
	if Engine.is_editor_hint():
		bone_template.bone_size = get_bone_count()

		var count = get_bone_count()
		for bone in count:
			bone_template.set_bone_name(bone,get_bone_name(bone))
			bone_template.set_bone_parent(bone,get_bone_name(get_bone_parent(bone)))
			bone_template.set_reference_pose(bone,get_bone_global_pose(bone))
			bone_template.set_handle_offset(bone,Vector2(.5,.1 +(bone *  .1)))
			# THe following command or something like it:
			#bone_template.set_require(bone,true)

Minimal reproduction project (MRP)

SkelProfile.zip

@AThousandShips
Copy link
Member

AThousandShips commented Mar 26, 2024

They're exposed as properties, you can modify them in the inspector I think, this might be documented better

We could expose them but unsure why they aren't and if there's some specific reason not to

@catprisbrey
Copy link
Author

catprisbrey commented Mar 26, 2024

They're definitely set-able in the inspector. But again, it's a pain with some skeletons i have plans on building templates for. Going through 30-50 bones clicking 'require' when everything else can be set in a loop, it's frustrating (not all will be required but it's easier to set all to required, and uncheck the extras rather than the other way around). It feels more like an oversight in a infrequently used resource rather than something intentionally not available in code.

I tried a bunch of methods yesterday trying to set it as a property instead and couldn't find a way that worked.

@AThousandShips
Copy link
Member

AThousandShips commented Mar 26, 2024

To set it as a property use:

bone_template.set("bones/i/require", true)

With i being the index of the bone, we'll see if there's any reason this wasn't exposed and I'll go ahead and expose it if there's no reason against it

@catprisbrey
Copy link
Author

catprisbrey commented Mar 26, 2024

I was about to pop in and say "my fresh morning coffee brain figured out how to set it" for my code."

bone_template.set("bones/" +str(bone) + "/require",true)

haha, and i popped in to edit my comment and you were already present with the same answer.

@AThousandShips
Copy link
Member

It was added along with several other of these methods two years ago but this one wasn't bound, there wasn't any reason mentioned, but don't assume it was an oversight and somewhat internal

CC @TokageItLab

@AThousandShips AThousandShips changed the title SkeletonProfile's is_require(), and set_require() aren't functional SkeletonProfile's is_require(), and set_require() aren't exposed Mar 26, 2024
@TokageItLab
Copy link
Member

TokageItLab commented Mar 28, 2024

I faintly remember avoiding binding them as actual properties because SkeletonProfileHumanoid (which extended class of SkeletonProfile) is always readonly and I don't want those values to be processed from the inspector (maybe I remember there was a problem with the array inspector), but I'm not sure now, sorry.

If we can handle readonly in a more correct way and ensure that the values of SkeletonProfileHumanoid's properties are immutable, I would never be opposed to binding them.

Well, it might not be a problem to expose a method, aside from exposing it as a property.

@AThousandShips
Copy link
Member

Then I'll expose them as a normal set of methods like the rest :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants