Re: wined3d: Fixed render states

[H. Verbeet]

On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> wined3d: Fixed up render states POINTSCALEENABLE, POINTSPRITENABLE,
> EDGEANTIALIAS, MULTISAMPLEANTIALIAS
> You probably want to split the patch up in smaller pieces.
> +        if(This->stateBlock->renderState[D3DRS_POINTSCALEENABLE]) {
> +            att[0] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_A]) /
> +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> +            att[1] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_B]) /
> +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> +            att[2] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_C]) /
> +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> +        }
> You're not supposed to use D3DRS_ stuff in wined3d, rather use the
> respective WINED3DRS_ defines, as the original code did.
> +        if(GL_SUPPORT(ARB_POINT_PARAMETERS)) {
> Same problem as with your patch for d3d8, wined3d doesn't currently
> check for ARB_POINT_PARAMETERS support (in IWineD3DImpl_FillGLCaps).
> -                /* Doesn't work with GL_POINT_SMOOTH on on my ATI 9600, but then ATI drivers are buggered! */
> -                glDisable(GL_POINT_SMOOTH);
> Did you verify that this isn't required anymore? Otherwise that's a
> potential regression.
> +            checkGLcall("glEnable(Gl_LINE_SMOOTH)");
> +            checkGLcall("glDisable(Gl_LINE_SMOOTH)");
> That should probably say Gl_LINE_SMOOTH, but it's not really critical.

wined3d: Fixed render states

[Vitaly Budovski]

H. Verbeet wrote:
> On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> > wined3d: Fixed up render states POINTSCALEENABLE, POINTSPRITENABLE,
> > EDGEANTIALIAS, MULTISAMPLEANTIALIAS
>
> You probably want to split the patch up in smaller pieces.

How would I do this?
> > +        if(This->stateBlock->renderState[D3DRS_POINTSCALEENABLE]) {
> > +            att[0] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_A]) /
> > +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> > +            att[1] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_B]) /
> > +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> > +            att[2] = *((float*)&This->stateBlock->renderState[D3DRS_POINTSCALE_C]) /
> > +                (This->stateBlock->viewport.Height * This->stateBlock->viewport.Height);
> > +        }
>
> You're not supposed to use D3DRS_ stuff in wined3d, rather use the
> respective WINED3DRS_ defines, as the original code did.

Typo, I'll fix that.
> > +        if(GL_SUPPORT(ARB_POINT_PARAMETERS)) {
>
> Same problem as with your patch for d3d8, wined3d doesn't currently
> check for ARB_POINT_PARAMETERS support (in IWineD3DImpl_FillGLCaps).

Will be submitted as a separate patch. Falls back to the EXT version for 
now.
> > -                /* Doesn't work with GL_POINT_SMOOTH on on my ATI 9600, but then ATI drivers are buggered! */
> > -                glDisable(GL_POINT_SMOOTH);
>
> Did you verify that this isn't required anymore? Otherwise that's a
> potential regression.

This seems to be a bug with (some?) ATI hardware/drivers.
Point smoothing should normally be supported I believe.
> > +            checkGLcall("glEnable(Gl_LINE_SMOOTH)");
> > +            checkGLcall("glDisable(Gl_LINE_SMOOTH)");
>
> That should probably say Gl_LINE_SMOOTH, but it's not really critical

Non-critical typo, but will be fixed when I resend the patch.

wined3d: Fixed render states

[H. Verbeet]

On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> H. Verbeet wrote:
> > On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> > > wined3d: Fixed up render states
> >
> > You probably want to split the patch up in smaller pieces.
>
> How would I do this?
> The changes to eg. EDGEANTIALIAS and POINTSPRITENABLE don't depend on
> eachother, and aren't really related, except for both of them being
> renderstates, so you could probably split those changes up into
> separate patches.
> > Same problem as with your patch for d3d8, wined3d doesn't currently
> > check for ARB_POINT_PARAMETERS support (in IWineD3DImpl_FillGLCaps).
>
> Will be submitted as a separate patch. Falls back to the EXT version for
> now.
> I think it is preferrable to check in working code as much as
> possible. Although it doesn't directly break stuff, it would probably
> be best to submit the checks for ARB_POINT_PARAMETERS support before
> trying to use it.
> > > -                /* Doesn't work with GL_POINT_SMOOTH on on my ATI 9600, but then ATI drivers are buggered! */
> > > -                glDisable(GL_POINT_SMOOTH);
> >
> > Did you verify that this isn't required anymore? Otherwise that's a
> > potential regression.
>
> This seems to be a bug with (some?) ATI hardware/drivers.
> Point smoothing should normally be supported I believe.
> Well yes, but I doubt we want to break stuff that was previously working.

wined3d: Fixed render states

[Vitaly Budovski]

H. Verbeet wrote:
> On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> > H. Verbeet wrote:
> > > On 31/01/06, Vitaly Budovski <vbudovsk(a)cs.rmit.edu.au> wrote:
> > > > wined3d: Fixed up render states
> > >
> > > You probably want to split the patch up in smaller pieces.
> >
> > How would I do this?
>
> The changes to eg. EDGEANTIALIAS and POINTSPRITENABLE don't depend on
> eachother, and aren't really related, except for both of them being
> renderstates, so you could probably split those changes up into
> separate patches.
> > > Same problem as with your patch for d3d8, wined3d doesn't currently
> > > check for ARB_POINT_PARAMETERS support (in IWineD3DImpl_FillGLCaps).
> >
> > Will be submitted as a separate patch. Falls back to the EXT version for
> > now.
>
> I think it is preferrable to check in working code as much as
> possible. Although it doesn't directly break stuff, it would probably
> be best to submit the checks for ARB_POINT_PARAMETERS support before
> trying to use it.
> > > > -                /* Doesn't work with GL_POINT_SMOOTH on on my ATI 9600, but then ATI drivers are buggered! */
> > > > -                glDisable(GL_POINT_SMOOTH);
> > >
> > > Did you verify that this isn't required anymore? Otherwise that's a
> > > potential regression.
> >
> > This seems to be a bug with (some?) ATI hardware/drivers.
> > Point smoothing should normally be supported I believe.
>
> Well yes, but I doubt we want to break stuff that was previously working.

Some more information confirming that it is indeed a problem with some 
ATI cards/drivers: 
http://www.openscenegraph.org/index.php?page=Tasks.OpenGLConformance

It would be good if someone with an ATI card could download the 
point_sprite_bug sample mentioned on that page and test it out to see if 
it is still an issue.
>

wined3d: Fixed render states

[Stefan Dösinger]

Hi,
> It would be good if someone with an ATI card could download the
> point_sprite_bug sample mentioned on that page and test it out to see if
> it is still an issue.
> I tested it with Mesa r200, my card is a radeon mobility 9000, but it gave me 
> a "GL_ARB_point_parameters extension was not found" error. I don't have fglrx 
> installed right now, so I can't test it

wined3d: Fixed render states

[Molle Bestefich]

Stefan Dösinger wrote:
> > It would be good if someone with an ATI card could download the
> > point_sprite_bug sample mentioned on that page and test it out to see if
> > it is still an issue.
>
> a "GL_ARB_point_parameters extension was not found" error.

Same here, Radeon 8500LE and open source drivers.