[PATCH v2] d3d10: Implement ID3D10EffectScalarVariable::{Get, Set}{Int, Float, Bool}
Signed-off-by: Nick Renieris <velocityra(a)gmail.com> --- dlls/d3d10/d3d10_private.h | 11 +++++++ dlls/d3d10/effect.c | 71 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/dlls/d3d10/d3d10_private.h b/dlls/d3d10/d3d10_private.h index e785b8b..4f51bf7 100644 --- a/dlls/d3d10/d3d10_private.h +++ b/dlls/d3d10/d3d10_private.h @@ -100,6 +100,16 @@ struct d3d10_effect_shader_variable } shader; }; +struct d3d10_effect_scalar_variable +{ + union + { + int i; + float f; + BOOL b; + }; +}; + struct d3d10_effect_state_object_variable { union @@ -173,6 +183,7 @@ struct d3d10_effect_variable { struct d3d10_effect_state_object_variable state; struct d3d10_effect_shader_variable shader; + struct d3d10_effect_scalar_variable scalar; } u; }; diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c index cd6f4a3..81b0a8a 100644 --- a/dlls/d3d10/effect.c +++ b/dlls/d3d10/effect.c @@ -4125,17 +4125,33 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetRawValue(ID3D10 static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloat(ID3D10EffectScalarVariable *iface, float value) { - FIXME("iface %p, value %.8e stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %.8e\n", iface, value); + + if (!iface) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + This->u.scalar.f = value; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetFloat(ID3D10EffectScalarVariable *iface, float *value) { - FIXME("iface %p, value %p stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %p\n", iface, value); + + if (!iface || !value) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + *value = This->u.scalar.f; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloatArray(ID3D10EffectScalarVariable *iface, @@ -4157,17 +4173,32 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetFloatArray(ID3D static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetInt(ID3D10EffectScalarVariable *iface, int value) { - FIXME("iface %p, value %d stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %d\n", iface, value); + + if (!iface) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + This->u.scalar.i = value; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetInt(ID3D10EffectScalarVariable *iface, int *value) { - FIXME("iface %p, value %p stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %p\n", iface, value); + if (!iface || !value) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + *value = This->u.scalar.i; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetIntArray(ID3D10EffectScalarVariable *iface, @@ -4189,17 +4220,33 @@ static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetIntArray(ID3D10 static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetBool(ID3D10EffectScalarVariable *iface, BOOL value) { - FIXME("iface %p, value %d stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %d\n", iface, value); + + if (!iface) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + This->u.scalar.b = value; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_GetBool(ID3D10EffectScalarVariable *iface, BOOL *value) { - FIXME("iface %p, value %p stub!\n", iface, value); + struct d3d10_effect_variable *This; - return E_NOTIMPL; + TRACE("iface %p, value %p\n", iface, value); + + if (!iface || !value) + return D3DERR_INVALIDCALL; + + This = impl_from_ID3D10EffectVariable((ID3D10EffectVariable *)iface); + *value = This->u.scalar.b; + + return S_OK; } static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetBoolArray(ID3D10EffectScalarVariable *iface, -- 2.7.4
Hi Nick, Thank you for the patch. I do have a few comments though: On Mon, 11 Mar 2019 at 19:40, Nick Renieris <velocityra(a)gmail.com> wrote:
+struct d3d10_effect_scalar_variable +{ + union + { + int i; + float f; + BOOL b; + }; +}; Please avoid nameless unions.
static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloat(ID3D10EffectScalarVariable *iface, float value) { - FIXME("iface %p, value %.8e stub!\n", iface, value); + struct d3d10_effect_variable *This;
I know a bunch of other code in the same file also does this, but please don't name variables "This". This may be one of those rare cases where "variable" would be a perfectly reasonable name, or perhaps simply "v".
+ if (!iface) + return D3DERR_INVALIDCALL; This check shouldn't be needed.
It would be great if you could write some tests to go along with this patch. Also, this patch implements 6 different functions; please split it so that each patch only makes a single conceptual change.
Hi, Στις Τρί, 12 Μαρ 2019 στις 1:25 μ.μ., ο/η Henri Verbeet <hverbeet(a)gmail.com> έγραψε:
On Mon, 11 Mar 2019 at 19:40, Nick Renieris <velocityra(a)gmail.com> wrote:
+struct d3d10_effect_scalar_variable +{ + union + { + int i; + float f; + BOOL b; + }; +}; Please avoid nameless unions.
Ok, I'll make `d3d10_effect_scalar_variable` a union.
static HRESULT STDMETHODCALLTYPE d3d10_effect_scalar_variable_SetFloat(ID3D10EffectScalarVariable *iface, float value) { - FIXME("iface %p, value %.8e stub!\n", iface, value); + struct d3d10_effect_variable *This;
I know a bunch of other code in the same file also does this, but please don't name variables "This". This may be one of those rare cases where "variable" would be a perfectly reasonable name, or perhaps simply "v".
"This" is indeed used everywhere. Can you explain what's different in my case?
It would be great if you could write some tests to go along with this patch. Also, this patch implements 6 different functions; please split it so that each patch only makes a single conceptual change.
I have no issue with doing that, but is it _really_ necessary for something so simple? If so, can I split to 3 commits, grouping Set/Get functions together? Thanks, Nick.
On Tue, Mar 12, 2019 at 11:02 PM Nick Renieris <velocityra(a)gmail.com> wrote:
I know a bunch of other code in the same file also does this, but please don't name variables "This". This may be one of those rare cases where "variable" would be a perfectly reasonable name, or perhaps simply "v".
"This" is indeed used everywhere. Can you explain what's different in my case?
"This" is used in the old code. We don't use "This" anymore in D3D code.
It would be great if you could write some tests to go along with this patch. Also, this patch implements 6 different functions; please split it so that each patch only makes a single conceptual change.
I have no issue with doing that, but is it _really_ necessary for something so simple? If so, can I split to 3 commits, grouping Set/Get functions together?
I think that 6 commits are preferred. It should result in a cleaner commit messages, i. e. "Implement ID3D10EffectScalarVariable::GetInt()." Also, tests would be useful. Your current implementation might be incorrect. Is it legal to call SetFloat() on boolean variables?
On Wed, Mar 13, 2019, 10:15 AM Józef Kucia <joseph.kucia(a)gmail.com> wrote:
"This" is used in the old code. We don't use "This" anymore in D3D code. Στις Τετ, 13 Μαρ 2019 στις 4:25 μ.μ., ο/η Henri Verbeet <hverbeet(a)gmail.com> έγραψε: And more broadly, uppercase or mixed case variable names.
Cool. The style did seem out of place.
It would be great if you could write some tests to go along with this patch. Also, this patch implements 6 different functions; please split it so that each patch only makes a single conceptual change.
I have no issue with doing that, but is it _really_ necessary for something so simple? If so, can I split to 3 commits, grouping Set/Get functions together?
I think that 6 commits are preferred. It should result in a cleaner commit messages, i. e. "Implement ID3D10EffectScalarVariable::GetInt()."
Also, tests would be useful. Your current implementation might be incorrect.
My bad, I was only referring the commit separation comment, not the tests. I would consider even > 1 commit separation an utter waste of my time and yours. I am and have worked on many open-source projects, and never come across this. Anyway, if those are the rules, so be it.
Is it legal to call SetFloat() on boolean variables?
As far as I'm aware the tests test against hardcoded values and not real D3D dlls, so how would a test help with that, if I don't even _know_ what the real library does (and I'm not allowed to reverse it + the docs don't explicitly mention)? The only thing I can think of is to write a test against the real D3D dlls or take a leap of faith and either do nothing, or return D3DERR_INVALIDCALL.
On Wed, Mar 13, 2019 at 7:05 PM Nick Renieris <velocityra(a)gmail.com> wrote:
Is it legal to call SetFloat() on boolean variables?
As far as I'm aware the tests test against hardcoded values and not real D3D dlls, so how would a test help with that, if I don't even _know_ what the real library does (and I'm not allowed to reverse it + the docs don't explicitly mention)? The only thing I can think of is to write a test against the real D3D dlls or take a leap of faith and either do nothing, or return D3DERR_INVALIDCALL.
We run all our tests against Windows DLLs. The tests should pass on Windows and on Wine. You may want to get a test bot account to run your tests on our VMs [1]. As a side note, I expect that the correct behavior is to convert float values to boolean values instead of doing data reinterpretation on the byte level. [1] - https://wiki.winehq.org/Wine_TestBot
participants (3)
-
Henri Verbeet -
Józef Kucia -
Nick Renieris