Horde3D

Next-Generation Graphics Engine
It is currently 20.06.2019, 10:15

All times are UTC + 1 hour




Post new topic Reply to topic  [ 5 posts ] 
Author Message
PostPosted: 10.09.2013, 14:56 
Offline

Joined: 08.01.2008, 14:57
Posts: 66
Hi guys,

I propose to add some new API functions relative to animation area.

For example, currently it looks impossible to retrieve info about the current weight and elapsed time of an animation.

There's a "h3dSetModelAnimParams(...)" function but not a "h3dgetModelAnimParams()" one.

My proposal is to do the following change:

Code:
/** Deprecate but still keep it for retro-compatibility. **/
void h3dSetModelAnimParams(H3DNode modelNode, int stage, float weight, float time);

/** Add new setter/getter functions. **/
void h3dSetModelAnimParamF(H3DNode modelNode, int stage, int paramIdx, float value);
float h3dGetModelAnimParamF(H3DNode modelNode, int stage, int paramId);

/** Add a new enum. **/
struct H3DAnimParams
{
   enum List
   {
      Weight = 1,
      ElapsedTime,
   };
};


We can still keep the old "h3dSetModelAnimParams" for retro-compatibility but deprecating its use (and remove it in a future version). This way the set/get process is consistent with the rest of the API.

Another plus is that we could add more functions to cover other types of params (int, str, etc.) if we need them.


Top
 Profile  
Reply with quote  
PostPosted: 10.09.2013, 17:43 
Offline
Tool Developer

Joined: 13.11.2007, 11:07
Posts: 1149
Location: Germany
I agree but would propose to break compatibility to not clutter up the API. Although the version names of Horde3D are not optimal (as discussed before), one could see the Beta version number as version minor number, and I don't have a problem with changing APIs slightly when changing the minor version.


Top
 Profile  
Reply with quote  
PostPosted: 13.09.2013, 12:17 
Offline

Joined: 08.01.2008, 14:57
Posts: 66
I changed a little bit my original intention with the following approach (which has also less impact on the API):

Code:
/** Add a few new entries to "H3DModel" enum. **/
struct H3DModel
{
    enum List
    {
        // ...
        AnimStageCountI,   // ... [read-only]
        AnimStageTimeF,
        AnimStageWeightF,
    }
}

/** Deprecate old "h3dSetModelAnimParams" API function. **/


Basically, that's it! :)

Adding these new values, let us to use existing interface to query for these parameters.

For example:

Code:
H3DHandle model;
int stage = 1;

/** Query for the elapsed time of stage 1 of the current model. **/
float time = h3dGetNodeParamF(model, H3DModel::AnimStageTimeF, stage);

/** Advance animation time. **/
h3dSetNodeParamF(model, H3DModel::AnimStageTimeF, stage, time + dt);

/** Change weight value. **/
h3dSetNodeParamF(model, H3DModel::AnimStageWeightF, stage, 0.5f);

/** Retrieve the number of active stages for the current model. **/
int active_stages = h3dGetNodeParamI(model, H3DModel::AnimStageCountI);


Obviously, changes to the internal API are necessary (to "Model" and "AnimationController" classes).

What do you think about this solution?

PS: A question. Why "Model" doesn't inherit from (or, better, why doesn't integrate the API of) "AnimationController"? What's the point to have this level of indirection?


Top
 Profile  
Reply with quote  
PostPosted: 13.09.2013, 20:16 
Offline
Tool Developer

Joined: 13.11.2007, 11:07
Posts: 1149
Location: Germany
I merged your pull request, but I'm not sure if the proposed patch is the best solution. The h3dSetModelAnimParams could be called very often (e.g. the Chicago sample). Having it split up into three function calls all doing the same validation on the passed parameters, seems not the best solution to me. Maybe we should add the getters the way you prosed it in your pull request, but leave the h3dSetModelAnimParams. Having both ways (setter functions and the separate h3dSetModelAnimParams function) may be confusing. What do others think?


Top
 Profile  
Reply with quote  
PostPosted: 16.09.2013, 10:11 
Offline

Joined: 08.01.2008, 14:57
Posts: 66
The problem is that both solutions could be "confusing":

1) Leave "h3dSetModelAnimParams" together with new setters: redundant (Current solution).

2) Remove setters but leave the new param values for getters: really inconsistent (the enum values are there but you can not use them with usual "h3dSetNodeParamF").

IMHO, the best solution is the first one. Redundancy is better than inconsistency. We can simply document that, due to performance reasons, is always preferable use "h3dSetModelAnimParams" over normal setters when you have to set both params at the same time (that's why I "removed" the deprecation of "h3dSetModelAnimParams").

EDIT: There's also a 3) option:

3) Remove new "AnimTimeF" and "AnimWeightF" values from "H3DModel" enum, but keep new "AnimCountI". Then add a new "void h3dGetModelAnimParams(H3DNode model, int stage, float* time, float* weight)" function to retrieve both parameters with one function call (in this case there is no big performance gain on the getter, but we keep the consistency between setter and getter without doubling the overhead on setter as in the current implementation). This is the same approach of "h3dGetNodeTransform(...)" function, for example.


Top
 Profile  
Reply with quote  
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 5 posts ] 

All times are UTC + 1 hour


Who is online

Users browsing this forum: No registered users and 3 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Jump to:  
cron
Powered by phpBB® Forum Software © phpBB Group