Re: [PATCH] d3dx9: IsParameterUsed stub should return TRUE.
On 7 March 2016 at 22:49, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Yes, I guess returning TRUE is usually more likely to work. Of course Is that necessarily what you want though? It might be harder to debug something being slow because it's updating unused parameters than to debug something rendering incorrectly because it's not updating them.
a proper implementation would be even better (although it may be non obvious to find out if a parameter is used, depending on what "used" really means - it needs tests)... You always need tests, but a reasonable guess would be whether the parameter is referenced by the technique.
2016-03-07 23:09 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>:
On 7 March 2016 at 22:49, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
Yes, I guess returning TRUE is usually more likely to work. Of course Is that necessarily what you want though? It might be harder to debug something being slow because it's updating unused parameters than to debug something rendering incorrectly because it's not updating them.
There is still the FIXME() and it shouldn't be impossible to figure out if there are redundant parameter updates. OTOH I guess failing to draw correctly is a much bigger incentive to implement the function properly... I don't know, I don't feel too strongly either way.
a proper implementation would be even better (although it may be non obvious to find out if a parameter is used, depending on what "used" really means - it needs tests)... You always need tests, but a reasonable guess would be whether the parameter is referenced by the technique.
That's likely to be the case, but yeah - tests.
On 03/08/2016 01:42 AM, Matteo Bruni wrote:
a proper implementation would be even better (although it may be non obvious to find out if a parameter is used, depending on what "used" really means - it needs tests)... You always need tests, but a reasonable guess would be whether the parameter is referenced by the technique. That's likely to be the case, but yeah - tests.
I actually already got real implementation for this function (here inside a big & dirty cumulative patch https://bugs.winehq.org/attachment.cgi?id=53869), but this requires testing as you say. My motivation to change the stub at this point was that it currently preventing testing the other bigger parts of effect framework on real applications, and real implementation will be easier test and add later. Since I can't add alltogether, I need to choose what to deal with first, and I thought it might be easier to verify a bigger piece when I get them working in real life rather then putting all the blocks in before getting anything real working.
2016-03-08 0:22 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/08/2016 01:42 AM, Matteo Bruni wrote:
a proper implementation would be even better (although it may be non obvious to find out if a parameter is used, depending on what "used" really means - it needs tests)... You always need tests, but a reasonable guess would be whether the parameter is referenced by the technique. That's likely to be the case, but yeah - tests.
I actually already got real implementation for this function (here inside a big & dirty cumulative patch https://bugs.winehq.org/attachment.cgi?id=53869), but this requires testing as you say. My motivation to change the stub at this point was that it currently preventing testing the other bigger parts of effect framework on real applications, and real implementation will be easier test and add later. Since I can't add alltogether, I need to choose what to deal with first, and I thought it might be easier to verify a bigger piece when I get them working in real life rather then putting all the blocks in before getting anything real working.
I get your point, that's why I signed off your patch :) Henri's point was also valid though. The other option was to not include this patch upstream (keeping this patch or your work-in-progress implementation in your private tree for testing purposes) and only submit the final IsParameterUsed() implementation when it's ready. In the end this means I'm trusting you to submit a proper implementation with tests later on.
On 03/08/2016 09:08 PM, Matteo Bruni wrote:
I get your point, that's why I signed off your patch :) Henri's point was also valid though. The other option was to not include this patch upstream (keeping this patch or your work-in-progress implementation in your private tree for testing purposes) and only submit the final IsParameterUsed() implementation when it's ready. In the end this means I'm trusting you to submit a proper implementation with tests later on. Thanks, I will do it after we finish preshaders. I am going to set first version of this patchset soon (tomorrow as latest). BTW while testing preshader math operation result I found a few mismatches between different Windows versions (32 bit and 64 bit actually). Could you please recommend a right way to deal with it in Wine tests?
2016-03-08 19:16 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/08/2016 09:08 PM, Matteo Bruni wrote:
I get your point, that's why I signed off your patch :) Henri's point was also valid though. The other option was to not include this patch upstream (keeping this patch or your work-in-progress implementation in your private tree for testing purposes) and only submit the final IsParameterUsed() implementation when it's ready. In the end this means I'm trusting you to submit a proper implementation with tests later on. Thanks, I will do it after we finish preshaders. I am going to set first version of this patchset soon (tomorrow as latest). BTW while testing preshader math operation result I found a few mismatches between different Windows versions (32 bit and 64 bit actually). Could you please recommend a right way to deal with it in Wine tests?
It depends but, if it has to do with float results precision, probably just allowing for a little tolerance is fine (see the compare_float() in d3dx9_36/tests/effect.c).
On 03/08/2016 09:32 PM, Matteo Bruni wrote:
2016-03-08 19:16 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/08/2016 09:08 PM, Matteo Bruni wrote: It depends but, if it has to do with float results precision, probably just allowing for a little tolerance is fine (see the compare_float() in d3dx9_36/tests/effect.c). One case can be solved by the tolerance (though I did not want to introduce it as the other cases match exactly), but the two others are about sin and cos from FLOAT_MAX value, and the difference is big. It is BTW a sort of msvcrt issue in my understanding. My (Wine) results match exactly native d3dx if Wine's msvcrt is used by native d3dx, and do not match when d3dx is using native msvcrt (and there is also the difference for that between Windows version). Native msvcrt is apparently using one of many possible tricky sin/cos/sqrt computation in Win32. I was curious if there is a legitimate Windows version dependent "wine_if", or some check for Wine version, or better exclude such test at all from comparison?
2016-03-08 19:39 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/08/2016 09:32 PM, Matteo Bruni wrote:
2016-03-08 19:16 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
On 03/08/2016 09:08 PM, Matteo Bruni wrote: It depends but, if it has to do with float results precision, probably just allowing for a little tolerance is fine (see the compare_float() in d3dx9_36/tests/effect.c). One case can be solved by the tolerance (though I did not want to introduce it as the other cases match exactly), but the two others are about sin and cos from FLOAT_MAX value, and the difference is big. It is BTW a sort of msvcrt issue in my understanding. My (Wine) results match exactly native d3dx if Wine's msvcrt is used by native d3dx, and do not match when d3dx is using native msvcrt (and there is also the difference for that between Windows version). Native msvcrt is apparently using one of many possible tricky sin/cos/sqrt computation in Win32. I was curious if there is a legitimate Windows version dependent "wine_if", or some check for Wine version, or better exclude such test at all from comparison?
Oh, interesting. You can use broken() to mark the Windows result that you don't want to replicate. In this case though not running the test may be okay too, given that there might not be a "correct" result. If you go for the latter, still put the affected test in a #if 0 block with a comment for documentation purposes.
participants (3)
-
Henri Verbeet -
Matteo Bruni -
Paul Gofman