Assuming that viewport_test in d3d9/tests/visual.c is wrong only when render_offscreen is true, is there anyway to fix that test? Or should viewport_test be removed?
The reason for the patch is that Overwatch renders to a 2D texture some UI elements. If the viewport is not what is expected, then the output is wrong (e.g. the UI elements are the wrong size or drawn at the wrong place on the texture).
-Andrew
On Sun, Nov 13, 2016 at 12:35 PM, Andrew Wesie awesie@gmail.com wrote:
Signed-off-by: Andrew Wesie awesie@gmail.com
dlls/wined3d/state.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-)
diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c index cd3763c..ed20e325 100644 --- a/dlls/wined3d/state.c +++ b/dlls/wined3d/state.c @@ -4617,26 +4617,6 @@ static void viewport_miscpart(struct wined3d_context *context, const struct wine struct wined3d_viewport vp = state->viewport; unsigned int width, height;
- if (target)
- {
if (vp.width > target->width)vp.width = target->width;if (vp.height > target->height)vp.height = target->height;wined3d_rendertarget_view_get_drawable_size(target, context,&width, &height);
- }
- else if (depth_stencil)
- {
width = depth_stencil->width;height = depth_stencil->height;- }
- else
- {
FIXME("No attachments draw calls not supported.\n");return;- }
- gl_info->gl_ops.gl.p_glDepthRange(vp.min_z, vp.max_z); checkGLcall("glDepthRange"); /* Note: GL requires lower left, DirectX supplies upper left. This is
@@ -4644,7 +4624,29 @@ static void viewport_miscpart(struct wined3d_context *context, const struct wine if (context->render_offscreen) gl_info->gl_ops.gl.p_glViewport(vp.x, vp.y, vp.width, vp.height); else
- {
if (target){if (vp.width > target->width)vp.width = target->width;if (vp.height > target->height)vp.height = target->height;wined3d_rendertarget_view_get_drawable_size(target, context,&width, &height);
}else if (depth_stencil){width = depth_stencil->width;height = depth_stencil->height;}else{FIXME("No attachments draw calls not supported.\n");return;}gl_info->gl_ops.gl.p_glViewport(vp.x, (height - (vp.y +vp.height)), vp.width, vp.height);
- } checkGLcall("glViewport");
}
-- 2.7.4
2016-11-18 0:58 GMT+01:00 Andrew Wesie awesie@gmail.com:
Assuming that viewport_test in d3d9/tests/visual.c is wrong only when render_offscreen is true, is there anyway to fix that test? Or should viewport_test be removed?
viewport_test() passes just fine currently.
The reason for the patch is that Overwatch renders to a 2D texture some UI elements. If the viewport is not what is expected, then the output is wrong (e.g. the UI elements are the wrong size or drawn at the wrong place on the texture).
Is that still an issue after 5cdb8f2486cf00a61c1aac20daef8c?
The point is that moving that block of code inside the !context->render_offscreen branch doesn't seem correct, the viewport width / height clamping should only depend on the d3d state while render_offscreen also depends on wined3d settings. By moving the code there you're effectively disabling it from wined3d with default settings. What is Overwatch depending on specifically? If it sets its viewport partially offscreen and doesn't like the clamping, we should probably make that chunk of code be (indirectly) conditional on the d3d version by means of a wined3d flag.
On Mon, Nov 21, 2016 at 6:51 PM, Matteo Bruni matteo.mystral@gmail.com wrote:
viewport_test() passes just fine currently.
I agree it does currently. If you play around with the viewport values though, then it will have differing behavior on real Windows and Wine. For instance, on my Windows machine, if I modify viewport_test to use a width and height of 1024x1024, then the behavior diverges from Wine.
When I tried different values for the viewport width and height in viewport_test, there seemed to be different behavior once the viewport width x height >= 2*1024*1024. For instance, a 1448x1448 viewport behaves differently than a 1449x1448 viewport. This behavior is likely dependent on the target size, which is 640x480 in the d3d9 tests.
The width and height values that are in viewport_test (10000 and 10000) seem arbitrary. Does that test reflect a real world program?
Is that still an issue after 5cdb8f2486cf00a61c1aac20daef8c?
Yes.
The point is that moving that block of code inside the !context->render_offscreen branch doesn't seem correct, the viewport width / height clamping should only depend on the d3d state while render_offscreen also depends on wined3d settings. By moving the code there you're effectively disabling it from wined3d with default settings.
I am trying to figure out why that block of code is even necessary. At the moment, it just appears to make viewport_test work, which is only useful if viewport_test reflects a real world program. My best guess was that the code was introduced to solve a problem when context->render_offscreen is false; hence I moved the code rather than eliminate it entirely.
What is Overwatch depending on specifically? If it sets its viewport partially offscreen and doesn't like the clamping, we should probably make that chunk of code be (indirectly) conditional on the d3d version by means of a wined3d flag.
Overwatch renders some UI elements to a texture that is smaller than the viewport. By clamping the viewport, the elements are now drawn incorrectly because you just changed the mapping from clip space to screen space.
I am not convinced that the original code is correct for any d3d version. The behavior I noted above with regards to testing different heights and widths demonstrates that pretty clearly. I can submit a patch to the test to make it clearer, if that is needed.
2016-11-22 5:48 GMT+01:00 Andrew Wesie awesie@gmail.com:
On Mon, Nov 21, 2016 at 6:51 PM, Matteo Bruni matteo.mystral@gmail.com
The width and height values that are in viewport_test (10000 and 10000) seem arbitrary. Does that test reflect a real world program?
Probably arbitrary values, yeah.
I am trying to figure out why that block of code is even necessary. At the moment, it just appears to make viewport_test work, which is only useful if viewport_test reflects a real world program. My best guess was that the code was introduced to solve a problem when context->render_offscreen is false; hence I moved the code rather than eliminate it entirely.
Somehow I spent most of the day looking into this. I extended the d3d9 test a bit and ran them on a couple of boxes here. It seems to me that, bugs with huge values aside, clamping the viewport values is never expected in d3d9. You're probably on point in that the code was there for the !render_offscreen case, specifically to get some sensible result from the (height - (vp.y + vp.height)) part. I still haven't tested other (in particular, older) d3d versions though.
What is Overwatch depending on specifically? If it sets its viewport partially offscreen and doesn't like the clamping, we should probably make that chunk of code be (indirectly) conditional on the d3d version by means of a wined3d flag.
Overwatch renders some UI elements to a texture that is smaller than the viewport. By clamping the viewport, the elements are now drawn incorrectly because you just changed the mapping from clip space to screen space.
I am not convinced that the original code is correct for any d3d version. The behavior I noted above with regards to testing different heights and widths demonstrates that pretty clearly. I can submit a patch to the test to make it clearer, if that is needed.
Yeah, that makes sense, my point was that it's never going to be right to clamp the viewport only in the !render_offscreen case, as you do in this patch: that flag is a wined3d "implementation detail" and not directly related to d3d state, application-visible behavior can't be dependent on it.
WRT affected wined3d code: aside from the viewport clamping and the viewport flipping in the !render_offscreen case, there is some (not critical) code related to depth buffer location / size tracking in wined3d which doesn't quite expect the viewport to be larger than the render target.
I'm going to cleanup my test patch and port the test to the other d3d versions, hopefully I'll have something useful in the next few days.
-
Andrew Wesie -
Matteo Bruni