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

GE:Bugfix of some backend. #12538

Closed
wants to merge 3 commits into from
Closed

GE:Bugfix of some backend. #12538

wants to merge 3 commits into from

Conversation

shenweip
Copy link
Contributor

This helps #12529.
List the graphics of all backend here:

Before:

  • opengl & vulkan:broken

1

  • D3D9:has an incorrect offset

2

  • D3D11:normal

3

After:

Do normal thing as d3d11.

@hrydgard hrydgard added this to the v1.10.0 milestone Dec 31, 2019
@hrydgard
Copy link
Owner

Cool! Looks like a good catch, I'm going to take a closer look, probably tomorrow, and see why we didn't update the viewport correctly before in this case...

@@ -562,6 +562,12 @@ void DrawEngineGLES::DoFlush() {
if (vertexCount > 0x10000 / 3)
vertexCount = 0x10000 / 3;
#endif
// Update viewport width & height before ST.
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we just moved SoftwareTransform(...) below ApplyDrawState?

It used to be that way (in fact that's exactly why there's even a ApplyDrawStateLate, which needed to run after), but it was moved in f99fa02 which might've mainly been about hardware transform.

-[Unknown]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,it works,but it's a little complicated in vulkan,because vulkan applies draw state as pipeline key.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, right. Seems like we should ultimately refactor this, probably putting the viewport info on SoftwareTransformParams and maybe no longer having an ApplyDrawStateLate...

-[Unknown]

@@ -342,7 +342,7 @@ void ShaderManagerDX9::VSUpdateUniforms(u64 dirtyUniforms) {
flippedMatrix[12] = -flippedMatrix[12];
}

ConvertProjMatrixToD3D(flippedMatrix, invertedX, invertedY);
ConvertProjMatrixToD3D(flippedMatrix, invertedX, !invertedY);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. If this is correct, I'd rather reverse the check inside the function rather than invert this value. This is the only call, and the argument inside and variable outside are both named invertedY.

I dislike the idea of them having opposite meanings - we should either inverse the check inside the func, or change one of the names, or reverse the way the variable is initially set here.

-[Unknown]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this more, is it a problem that this invert is in non-GL specific code?

if (useBufferedRendering)
ySign = -ySign;
float flippedMatrix[16];
if (!throughmode) {
memcpy(&flippedMatrix, gstate.projMatrix, 16 * sizeof(float));
const bool invertedY = useBufferedRendering ? (gstate_c.vpHeight < 0) : (gstate_c.vpHeight > 0);

(though I thought we used top down, or basically upside down, in GL buffers except the backbuffer...)

-[Unknown]

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that looks really broken...

@shenweip
Copy link
Contributor Author

Switching to #12596, closing this now.

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