Re: wined3d: Fixed render states

[H. Verbeet]

On 31/01/06, Vitaly Budovski  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  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  wrote:
> H. Verbeet wrote:
> > On 31/01/06, Vitaly Budovski  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  wrote:
> > H. Verbeet wrote:
> > > On 31/01/06, Vitaly Budovski  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.