-
Notifications
You must be signed in to change notification settings - Fork 867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[d3d9] Implement several IDirect3DShaderValidator9 validations #4398
base: master
Are you sure you want to change the base?
Conversation
I've had to comment out:
because on many occasions it seems like:
That causes the game to crash before getting to the main menu. @misyltoad I hope you have a better memory than I do and maybe have a clue what's going on here?
Nevermind, not even that does. The game crashes later on, so it seems like there's no guarantee that cdw and dwordLength are equal, though most of the time they are. |
39aa686
to
2ca49a5
Compare
@Blisto91 Temba, his arms wide. Seems to not crash for me now in the problematic parts. Please also test when you can. |
61e342b
to
76d7f2c
Compare
Left the BadInstructionLength validations commented out and with a TODO, since I'm not sure what's going on there. Perhaps it's something we can improve upon in the future, if any game actually cares. I'd welcome more testing, but at this stage it seems to be enough for The Void. The risk of breaking other stuff I think is rather minimal, since all other validations deal with shader type/structure validations and IDirect3DShaderValidator9 call order. |
76d7f2c
to
8105833
Compare
Act of War - Direct Action tries to use register input number 11 in a PS and expects that to work, otherwise it hangs before starting a game. Back to the drawing board, I guess. Edit: The game uses input register index 11 in a V1.1 PS. I guess I should actually restrict the check to PS 3.0, as per intended design, duh. |
8105833
to
8d5f9a1
Compare
8e2f2f3
to
899edb2
Compare
337926c
to
5154ae7
Compare
Should be good to go post a review. It's been tested with quite a few games, and at this point I'm fairly confident it shouldn't regress anything. That being said, it's d3d9, so we won't know until it's merged. |
|
||
// TODO: Consider switching this to debug, once we're | ||
// confident the implementation doesn't cause any issues | ||
Logger::warn(Message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we expect this to get called in every game that uses the validation functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only in case of actual validation errors, however some of these (like in the case of The Void) are expected behavior. It would be useful to see them in logs however, at least for a while, until we're sure it's not causing unintended validations in other places.
Testing so far has been promising, but you never know, since this is like entirely undocumented.
|
||
return D3D_OK; | ||
} | ||
|
||
|
||
HRESULT STDMETHODCALLTYPE End() { | ||
Logger::debug("D3D9ShaderValidator::End: Stub"); | ||
if (unlikely(m_state == D3D9ShaderValidatorState::Error)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the validator instance is not reusable if validation failed? Does this match native behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good question for @misyltoad . I'm afraid I have no idea how to go about testing this. Some games do seem to recreate the validator instance for each shader from what I've noticed, but I have no idea if this is intended by design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S.: According to these tests it does indeed seem that once a validation error is hit, the validator instance must be released and recreated. Great API, I know.
5154ae7
to
05658bf
Compare
Co-Authored-By: Joshua Ashton <[email protected]> Co-Authored-By: Paul Gofman <[email protected]>
05658bf
to
63c9339
Compare
Fixes #2045.
(Working) rebase of @misyltoad's former d3d9-shader-val2 branch, which doesn't crash on the main menu with The Void. On top of it I've added a crude adaptation of @gofman's PS input counts validation. It seems to work fine in terms of actually doing what's expected of it
, but it doesn't currently fix the crashing issues in the game.A save file that can reproduce the crashing can be found here.