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

Idea: replace animation channel target string with enum #279

Open
jdumont0201 opened this issue Jul 11, 2020 · 1 comment
Open

Idea: replace animation channel target string with enum #279

jdumont0201 opened this issue Jul 11, 2020 · 1 comment

Comments

@jdumont0201
Copy link

I really like how tinygltf defines constants to work with gltf properties. How about expanding it to animation channel targets too?

Right now, channel targets are accessible as target_path and directly read as std::string from gltf. They only take three possible values "translation","scale","rotation" and enums would allow more efficient comparisons when using it.

I use these edits in my project, keeping the original string value but adding a new enum field

enum TargetPathType
	{
		TINYGLTF_CHANNEL_TARGET_TRANSLATION,
		TINYGLTF_CHANNEL_TARGET_ROTATION,
		TINYGLTF_CHANNEL_TARGET_SCALE,
	};

Edit 1. The class definiton with an additional property and the original for compatiblity:

	
struct AnimationChannel {
        int sampler;              
        int target_node;       
        std::string target_path; 
        TargetPathType target_path_enum;  ///new code
        // ...

Edit 2. When parsing:

 if (!ParseStringProperty(&channel->target_path, err, target_object, "path",
                             true)) {
      if (err) {
        (*err) += "`path` field is missing in animation.channels.target\n";
      }
      return false;
    }
///new code
    if (*&channel->target_path == "translation") *&channel->target_path_enum = TINYGLTF_CHANNEL_TARGET_TRANSLATION;
    else if (*&channel->target_path == "rotation") *&channel->target_path_enum = TINYGLTF_CHANNEL_TARGET_ROTATION;
    else if (*&channel->target_path == "scale") *&channel->target_path_enum = TINYGLTF_CHANNEL_TARGET_SCALE;
@syoyo
Copy link
Owner

syoyo commented Jul 12, 2020

@jdumont0201 Good idea!

glTF 2.0 spec schema says

https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/schema/animation.channel.target.schema.json

so we'll need to define enum as:

enum TargetPathType
	{
		TINYGLTF_CHANNEL_TARGET_TRANSLATION,
		TINYGLTF_CHANNEL_TARGET_ROTATION,
		TINYGLTF_CHANNEL_TARGET_SCALE,
		TINYGLTF_CHANNEL_TARGET_WEIGHTS,
		TINYGLTF_CHANNEL_TARGET_STRING,  // e.g. for glTF extension?
	};

Then application can refer target_path string for TINYGLTF_CHANNEL_TARGET_STRING.

PR is appreciated!

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

No branches or pull requests

2 participants