Re: [PATCH 1/3] d3dx9: Implement D3DXCreateAnimationController with a stubbed ID3DXAnimationController interface. (try 2)

[Matteo Bruni]

2016-01-06 3:52 GMT+01:00 Alistair Leslie-Hughes :
> From: Christian Costa
>
> Correct QI to be like the IDevice11 one.
> Changed DOUBLE to double in the interface.
> Adjusted parameter names in D3DXCreateAnimationController
>
> Signed-off-by: Alistair Leslie-Hughes
> +static inline struct d3dx9_animation_controller *impl_from_ID3DXAnimationController(ID3DXAnimationController *iface)

Not a big deal but I think it's better to avoid the "inline" here,
it's unnecessary in this case since the function isn't in a .h file.
> +static HRESULT WINAPI d3dx9_animation_controller_AdvanceTime(ID3DXAnimationController *iface, double time_delta,
> -
>    ID3DXAnimationCallbackHandler **callback_handler)

I think callback_handler only wants one '*' here.
> +static HRESULT WINAPI d3dx9_animation_controller_GetTrackPriority(ID3DXAnimationController *iface,
> -
>    UINT track, D3DXPRIORITY_TYPE *priority)

This one should be called SetTrackPriority, at least according to the
header from the June 2010 DirectX SDK. The header currently in Wine
has the same GetTrackPriority prototype you used here (it's there
since the header was first introduced by Dylan Smith in
5cccad1311745117628b9a5f10652b5211151b51). I guess this is simply a
typo in our header and it "propagated" here but if anyone knows better
please speak up ;)

Quite reasonably, in a sense, the "priority" parameter for the
SetTrackPriority method is not supposed to be a pointer.
> +static HRESULT WINAPI d3dx9_animation_controller_SetTrackPosition(ID3DXAnimationController *iface,
> -
>    UINT track, double position)
>
> +{
> - FIXME("iface %p, track %u, position %g stub.\n", iface, track, position);

It doesn't seem a good choice to me to use %g instead of %f for double
parameters.
> +static HRESULT WINAPI d3dx9_animation_controller_SetTrackEnable(ID3DXAnimationController *iface,
> -
>    UINT track, BOOL enable)
>
> +{
> - FIXME("iface %p, track %u, enable %u stub.\n", iface, track, enable);

BOOL is defined as long i.e. signed.
> +static D3DXEVENTHANDLE WINAPI d3dx9_animation_controller_GetUpcomingPriorityBlend(ID3DXAnimationController *iface,
> -
>    D3DXEVENTHANDLE handle)
>
> +{
> - FIXME("iface %p, handle %u stub.\n", iface, handle);

Not too important but you called all the other occurrences "event"
instead of "handle".
> +static HRESULT WINAPI d3dx9_animation_controller_CloneAnimationController(ID3DXAnimationController *iface, UINT max_num_anim_outputs,
> -
>    UINT max_num_anim_sets, UINT max_num_tracks, UINT max_num_events, ID3DXAnimationController **anim_controller)

You can probably drop the "num_" part of those parameter names for brevity.

[PATCH 1/3] d3dx9: Implement D3DXCreateAnimationController with a stubbed ID3DXAnimationController interface. (try 2)

[Henri Verbeet]

On 8 January 2016 at 01:55, Matteo Bruni  wrote:
> > +static inline struct d3dx9_animation_controller *impl_from_ID3DXAnimationController(ID3DXAnimationController *iface)
>
> Not a big deal but I think it's better to avoid the "inline" here,
> it's unnecessary in this case since the function isn't in a .h file.

I don't necessarily disagree, but just about every other impl_from_*()
implementation does have "inline".
> > +static HRESULT WINAPI d3dx9_animation_controller_SetTrackPosition(ID3DXAnimationController *iface,
> > -
> >    UINT track, double position)
> >
> > +{
> > - FIXME("iface %p, track %u, position %g stub.\n", iface, track, position);
>
> It doesn't seem a good choice to me to use %g instead of %f for double
> parameters.

What you want is %a, but that's C99. But yeah, just stick with %e and
an appropriate precision specifier.
> > +static HRESULT WINAPI d3dx9_animation_controller_SetTrackEnable(ID3DXAnimationController *iface,
> > -
> >    UINT track, BOOL enable)
> >
> > +{
> > - FIXME("iface %p, track %u, enable %u stub.\n", iface, track, enable);
>
> BOOL is defined as long i.e. signed.

That doesn't necessarily mean it makes more sense to print it like
that though. Personally I'd go with %#x.

[PATCH 1/3] d3dx9: Implement D3DXCreateAnimationController with a stubbed ID3DXAnimationController interface. (try 2)

[Matteo Bruni]

2016-01-08 2:14 GMT+01:00 Henri Verbeet :
> On 8 January 2016 at 01:55, Matteo Bruni  wrote:
> > > +static inline struct d3dx9_animation_controller *impl_from_ID3DXAnimationController(ID3DXAnimationController *iface)
> >
> > Not a big deal but I think it's better to avoid the "inline" here,
> > it's unnecessary in this case since the function isn't in a .h file.
>
> I don't necessarily disagree, but just about every other impl_from_*()
> implementation does have "inline".
> > > +static HRESULT WINAPI d3dx9_animation_controller_SetTrackPosition(ID3DXAnimationController *iface,
> > > -
> > >    UINT track, double position)
> > >
> > > +{
> > > - FIXME("iface %p, track %u, position %g stub.\n", iface, track, position);
> >
> > It doesn't seem a good choice to me to use %g instead of %f for double
> > parameters.
>
> What you want is %a, but that's C99. But yeah, just stick with %e and
> an appropriate precision specifier.
> > > +static HRESULT WINAPI d3dx9_animation_controller_SetTrackEnable(ID3DXAnimationController *iface,
> > > -
> > >    UINT track, BOOL enable)
> > >
> > > +{
> > > - FIXME("iface %p, track %u, enable %u stub.\n", iface, track, enable);
> >
> > BOOL is defined as long i.e. signed.
>
> That doesn't necessarily mean it makes more sense to print it like
> that though. Personally I'd go with %#x.

Those are all good points. WRT the precision for the %e specifier, the
correct value for double (to avoid losing precision) would be ".16".
That's a lot of digits but nevertheless it seems preferable to
discarding potentially valuable information.