Skip to content
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

[d3d8] Validate viewport dimensions on SetViewport #4245

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

WinterSnowfall
Copy link
Contributor

@WinterSnowfall WinterSnowfall commented Sep 7, 2024

D3D8 apparently validates this, while D3D9 does not. I've checked that WineD3D also performs this validation, I am yet to check olden (WinXP) native behavior. Native d3d8 drivers, of WinXP fame, also perform this validation.

P.S.: @Blisto91 Also tested on modern Windows with AMD and it also performs the validation.

@WinterSnowfall WinterSnowfall marked this pull request as ready for review September 8, 2024 14:56
@doitsujin
Copy link
Owner

Does anything happen to existing viewports when the app binds a smaller render target?

@WinterSnowfall
Copy link
Contributor Author

Does anything happen to existing viewports when the app binds a smaller render target?

Guess it's worth a quick test, but I doubt that affects anything unless the app also sets a new viewport.

@WinterSnowfall
Copy link
Contributor Author

@doitsujin After setting a smaller sized render target, apparently we already automatically adjust the viewport to fit (at least with a following call to GetViewport its new dimensions are equal to the smaller render target). WineD3D behaves the same. So no need to worry about this bit, just games being silly with SetViewport as per above.

@WinterSnowfall WinterSnowfall marked this pull request as draft September 8, 2024 18:42
@WinterSnowfall
Copy link
Contributor Author

Hmmm... I hadn't considered the X and Y (upper left) coordinates in the validation actually, and some games might do that. I'll adjust and retest this.

@WinterSnowfall
Copy link
Contributor Author

Alright, this should be sufficient. Verified with WineD3D and native WinXP drivers that the validation also takes the X and Y offsets into account (which makes sense).

@WinterSnowfall WinterSnowfall marked this pull request as ready for review September 8, 2024 19:15
src/d3d8/d3d8_device.cpp Outdated Show resolved Hide resolved
src/d3d8/d3d8_device.cpp Show resolved Hide resolved
@WinterSnowfall WinterSnowfall force-pushed the d3d8-viewport branch 2 times, most recently from 7208343 to 5c08384 Compare September 12, 2024 14:09
Copy link
Collaborator

@K0bin K0bin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The m_renderTarget null check is technically unnecessary as far as I can tell but it doesn't hurt either so whatever.

@WinterSnowfall
Copy link
Contributor Author

Should fix the aspect ratio problem in #4246, so it fixes it for good in conjunction with #4258.

@K0bin K0bin merged commit 8e03b64 into doitsujin:master Sep 12, 2024
4 checks passed
@WinterSnowfall WinterSnowfall deleted the d3d8-viewport branch September 12, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants