On Mon Oct 10 00:52:11 2022 +0000, Patrick Hibbs wrote:
OK, fixed the commit subject. I've also added some tests. There will need to be another revision to this MR for wine to pass them correctly, but they currently pass without error on Win10. I do have a few questions:
- Is there a preferred method for testing byte-misaligned floats in
wine tests? Currently, I'm setting the unaffected parts of the array in the initializer, and then fixing it up at runtime with a memcpy(). Is there another method I should use? 2) As Win10 allows setting the byte-misaligned float, how should I deal with the assert in param_get_data_and_dirtify()? As the tests currently fail under wine due to it. 3) How did you get fx.exe to output DWORDs? I can manually clean it up but I'd prefer to have less to do. :smile: 4) Any other clean ups or tests to recommend?
First of all, sorry for the delay.
These are some good tests, in principle. They already show that the implementation is not correct (e.g. it doesn't sanitize D3DXPT_BOOL values to 0/1) but I guess you already knew that.
Actually, in regard to that point specifically: assuming that SetRawValue() does in fact do what it says, it suggests that BOOL values are stored unchanged and are instead sanitized on use (or e.g. by GetValue()). That's probably something that we want to fix in our implementation, separately from SetRawValue() itself. Marking the relevant tests as todo_wine initially and fixing them afterwards is an option.
The expected results used by the tests are muddied quite a bit by the fact that the parameter value is not cleared to 0 (or any other value) between individual tests. That also makes it more complicated than necessary to add more tests. I'd fix that by explicitly clearing the tested parameter before each test.
WRT your questions: 1. I think you can simply compute the expected results beforehand and put them into static const variables. You can certainly e.g. use your test_effect_setrawvalue_init_floats() function with some additional traces to figure out what those values are supposed to be, but I don't see any reason to recalculate the expected results at runtime in the final tests. It might make sense to have a comment in the test explaining what those "weird" expected results actually mean, hard to say at this point. 2. If I understand the test correctly, there is no evidence that SetRawValue() can "overflow" the current parameter. Either way, I think it's a matter of cutting the value size to the maximum of bytes and param->bytes or something like that. 3. For sure :smile:. I think we used the /Fx output parameter to get DWORD data. 4. Testing that SetRawValue() doesn't affect the "next" parameter when used with larger offsets / sizes is probably interesting. I also have a bunch of comments that I'll mention inline. There's most likely more, it's not an exhaustive list.
One more thing. I saw you just pushed a new version of the patches, fixing a couple issues I had noticed. Nice, but generally please wait to push updates while I have the MR assigned to me, it means I'm actively reviewing it and changing stuff at that time can be somewhat disruptive for me.