Message info
 
To:Anuj Phogat From:Paul Berry Subject:Re: [Piglit] [PATCH] Split accuracy test to allow new multisample tests utilize this code Date:Mon, 7 May 2012 13:50:24 -0700
 

On 7 May 2012 13:28, Anuj Phogat <anuj.phogat@gmail.com> wrote:
On Mon, May 7, 2012 at 11:53 AM, Paul Berry <stereotype441@gmail.com> wrote:
>
> On 4 May 2012 15:02, Anuj Phogat <anuj.phogat@gmail.com> wrote:
>>
>> This patch splits accuracy.cpp in to three files: common.h, common.cpp
>> and accuracy.c.
>>
>> common.cpp defines the functions which can be utilized to develop new
>> multisample test cases. Functions can be utilized to:
>> - Draw a test image to default framebuffer.
>> - Initialize a FBO with specified samples count.
>> - Draw a test image to FBO.
>> - Draw a reference image.
>> - Verify the accuracy of multisample antialiasing in FBO.
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
>
>
> It seems like you've gone to a bunch of effort to provide a C interface (rather than a C++ interface) between the accuracy test and common.cpp, so that the accuracy test can be written in straight C. Can you comment on the benefits of this?
I provided the C interface in sync with my plans to develop new
multisample test cases using C. There are no specific benefits of this
choice.

> I see two downsides: a. it forces us to introduce extra wrapper functions that manipulate global state, and that doesn't seem desirable. b. if we later decide to add a test that re-uses part of the common structure but not all of it (e.g. by adding a new TestPattern-derived class), we won't be able to do so easily.
>
I agree. That would require some extra efforts.

> Unless there's something I'm missing, I would prefer if we took an approach like this to sharing code:
>
> - Move the class declarations from accuracy.cpp into common.h.
> - Move the member function implementations from accuracy.cpp into common.cpp.
> - Leave the global variable Test *test = NULL; in accuracy.cpp, so that the global state is confined to the individual test file, and isn't part of the interface between accuracy and common.
> - Leave accuracy.cpp as a C++ program.
>
I initially re factored using a similar approach. But later thoughts
of providing a "C" interface overwhelmed me.
I am fine with your suggestion and comfortable developing new tests in C++.

> If you're interested, I'd be willing to take a stab at this approach and send it to the list.
>
Can you modify few member functions as per testing requirements of
turn-on-off.c ([PATCH] Add test to turn on/off MSAA in a FBO) ?
e.g define test_fbo, provide an option of rendering test image to
default fb, using render buffer as color attachment for 0 sample
count, re initializing test_fbo with specified sample_count.
I am fine with making above listed changes to re factored
accuracy.cpp, if you face any issues understanding exact requirements.

Ok, thanks. Since I objected to your refactor, I feel like it's only fair for me to put together an alternative. I'll get my proposal out to the list as soon as I can.