Re: [PATCH 3/5] wined3d: Make the condition to enter p8 conversion shader path more strict.
2008/8/31 Alexander Dorofeyev alexd4@inbox.lv:
This prevents shader path from being entered for an offscreen surface when there is p8 render target and fixes failures in ddraw visual test (with opengl rendering and RTL_READDRAW mode) and visual glitches in Red Alert.
dlls/wined3d/surface.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
This is probably ok for this specific case, but in general I think primary_render_target_is_p8() is somewhat broken. It seems that in most places where it's used it is assumed that "This" is the primary render target. In some cases that's actually always true, but in those cases primary_render_target_is_p8() wouldn't need to check if there's a primary render target, it could just use a surface passed in as parameter. In the other cases, you don't actually know the surface is the primary render target, and you'd need to verify that first, after which you've essentially got the first case again.
Henri Verbeet wrote:
2008/8/31 Alexander Dorofeyev alexd4@inbox.lv:
This prevents shader path from being entered for an offscreen surface when there is p8 render target and fixes failures in ddraw visual test (with opengl rendering and RTL_READDRAW mode) and visual glitches in Red Alert.
dlls/wined3d/surface.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
This is probably ok for this specific case, but in general I think primary_render_target_is_p8() is somewhat broken. It seems that in most places where it's used it is assumed that "This" is the primary render target. In some cases that's actually always true, but in those cases primary_render_target_is_p8() wouldn't need to check if there's a primary render target, it could just use a surface passed in as parameter. In the other cases, you don't actually know the surface is the primary render target, and you'd need to verify that first, after which you've essentially got the first case again.
I think that it's not that primary_render_target_is_p8() is broken, but things sometimes get confusing because there are two kinds of cases. Sometimes, we only want to care that primary is p8 (this is used as kind of indicator we are in 8 bit 2-D ddraw game and don't need to care about 3D requirements) and if so do something for all p8 surfaces. d3dfmt_p8_init_palette is one such place (index in alpha optimization).
But one case with different requirements seems to be the p8 conversion shader. I think as it is currently implemented, it is only meant to work for the p8 primary itself. But, because primary_render_target_is_p8() check was used in d3dfmt_get_conv, p8 conversion shader-specific paths were sometimes hit when they shouldn't.
2008/9/2 Alexander Dorofeyev alexd4@inbox.lv:
I think that it's not that primary_render_target_is_p8() is broken, but things sometimes get confusing because there are two kinds of cases. Sometimes, we only want to care that primary is p8 (this is used as kind of indicator we are in 8 bit 2-D ddraw game and don't need to care about 3D requirements) and if so do something for all p8 surfaces. d3dfmt_p8_init_palette is one such place (index in alpha optimization).
I think that's a rather hacky way to detect that. If that's what the function is supposed to do it should at the very least be named differently (to prevent this kind of bugs), but you should probably just check if the device is supposed to be 3D capable. I realize that's not possible yet and would need to be built, but we'll probably need this in other places as well.
-
Alexander Dorofeyev -
Henri Verbeet