[Piglit] [PATCH 1/2] arb_shader_stencil_export: move to standard location

[Emil Velikov]

On 10 November 2015 at 17:16, Ben Widawsky <benjamin.widawsky at intel.com> wrote:
> On Tue, Nov 10, 2015 at 04:37:09PM +0000, Emil Velikov wrote:
>> Hi Ben,
>>
>> Please feed these through -M so that git can detect the code movement.
>> Otherwise people start running away :P
>
> What does -M do? I am not seeing such a flag in any of the relevant git
> subcommands (add, rm, commit, mv). I thought git detects moves automatically,
> which is what it sounds like you're getting at.
>
It is here -M[n], --fine-renames[=<n>] for git show, format-patch
(should also work with send-email).

And yes internally git detects file movements/renames. Although for
compatibility with patch, I believe, it always does the full blown -
"old file removed, new one created".

With the above switch you'll see a ~10 line diff as opposed to
~200.... which definitely helps :-P

>>
>> But seriously, just a small comment below.
>>
>> On 23 October 2015 at 06:00, Ben Widawsky <benjamin.widawsky at intel.com> wrote:
>>
>> > --- /dev/null
>> > +++ b/tests/spec/arb_shader_stencil_export/glsl-fs-shader-stencil-export.c
>>
>> > +enum piglit_result
>> > +piglit_display(void)
>> > +{
>> > +       GLboolean pass = GL_TRUE;
>> > +       float p[4];
>> > +
>> > +       glClearColor(0.5, 0.5, 0.5, 0.5);
>> > +       glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
>> > +
>> > +       glEnable(GL_STENCIL_TEST);
>> > +       glStencilOp(GL_REPLACE, GL_REPLACE, GL_REPLACE);
>> > +
>> > +       piglit_draw_rect(-1, -1, 2, 2);
>> > +
>> > +       glReadPixels(0, 0, 3, 1, GL_STENCIL_INDEX, GL_FLOAT, p);
>>
>> The original test uses rect(0, 0 piglit_width, piglit_height) +
>> ortho_projection. As you've changed the former and dropped the latter
>> the glReadPixels() should be updated as well ?
>>
>
> I apologize for leaving the ortho change out of the commit message. It really
> should have been in there...
>
> I admit incompetence here, but I didn't think glReadPixels needs to be updated.
> Maybe I need a glViewport call... Could you enlighten me as to what it should
> be?
>
Don't think that one needs to tweak the viewport, but I'm not too sure
bth. Just that draw rect with x, y = -1 and width/height = 2 only to
ReadPixels x,y = 0, width/height 3/1 does not look correct.

Fwiw you can keep the original draw_rect + ortho_projection here and
then tweak things as a follow up once others chime in ? Up-to you
really.

>> You'd also need to update all.py. Currently it will attempt to run the
>> old, missing test. Something like
>>
>> diff --git a/tests/all.py b/tests/all.py
>> index c28ae2b..03ee52e 100644
>> --- a/tests/all.py
>> +++ b/tests/all.py
>> @@ -508,7 +508,6 @@ with profile.group_manager(PiglitGLTest, 'shaders') as g:
>>     g(['glsl-fs-pointcoord'])
>>     g(['glsl-fs-raytrace-bug27060'])
>>     g(['glsl-fs-sampler-numbering'])
>> -    g(['glsl-fs-shader-stencil-export'])
>>     g(['glsl-fs-sqrt-branch'])
>>     g(['glsl-fs-texturecube'])
>>     g(['glsl-fs-texturecube', '-bias'], 'glsl-fs-texturecube-bias')
>> @@ -2551,6 +2550,10 @@ with profile.group_manager(
>>         'GL_DEPTH32F_STENCIL8', 0)
>>
>> with profile.group_manager(
>> +        PiglitGLTest, grouptools.join('spec',
>> 'arb_shader_stencil_export')) as g:
>> +    g(['glsl-fs-shader-stencil-export'])
>> +
>> +with profile.group_manager(
>
> Sure. I can do this. Not sure how useful it is with just the one test, but the
> plan is to add more.
>
The point here is that we'll be actually have the test in the list (so
it's ran). And yes, the format is not the most obvious for a single
test, but in most cases we have many per extension.

>>
>> On the really nitpicky side one can rename the test to something
>> better, but I'm out of ideas :-\
>
> Eventually if I get a chance to add more tests, I will do that, but for now,
> laziness wins.
>
But of course. At that point you'll probably have some ideas for a the name.

Cheers,
Emil