[Mesa-dev] [PATCH 0/4] Removal of point size clamping in st/mesa

[Roland Scheidegger]

Am 29.01.2012 03:02, schrieb Marek Olšák:
> On Sat, Jan 28, 2012 at 2:37 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 28.01.2012 01:38, schrieb Marek Olšák:
>>> Hi everyone,
>>>
>>> the subject says it all. This series fixes gl_PointSize with transform feedback. There is a new piglit test to verify that a driver does clamping properly during rasterization: vs-point_size-zero
>>>
>>> I haven't changed Draw, because softpipe and llvmpipe do the clamping internally somewhere. (I didn't take a look where they do it, but they pass the test, which can't be said about r600 with point size clamping disabled)
>>>
>>> The only drivers I am not sure about are i915 and nouveau.
>>>
>>> Please review.
>>
>> This looks generally ok to me. I'd like to see more comment for [4/4],
>> e.g. why this is ok (something along the lines that drivers are expected
>> to clamp against their advertized point size limits whatever they are
>> and depending on point smooth etc.).
> 
> More comment where? In the commit message or in the code?
Commit messages is just fine.

> 
>> For [1/4] I can't quite tell off-hand if the point size min rs applies
>> everywhere only hope so...
>> I believe the conditions in [1,2,3/4] should also take multisampling
>> into account, since point rendering with multisampling also works
>> differently (and will produce zero-sized points).
>> Also maybe for the hw drivers there should be some comment why actually
>> min size is 1.0, as this is merely a workaround for chips which can't
>> follow legacy OGL's totally silly point rasterization rules (about as
>> silly as smooth points...).
> 
> I updated the patches, they can be read here:
> http://cgit.freedesktop.org/~mareko/mesa/log/?h=point-size-clamp
> 
> I added a helper function returning the expected minimum point size,
> it also checks for multisample and gl_rasterization_rules:
> http://cgit.freedesktop.org/~mareko/mesa/commit/?h=point-size-clamp&id=a57d828e924c3fbee1d4707c3cefe4a8b92deef6

Looks good to me. I think the language is somewhat unfortunate in some
places ("The point size should be clamped to this value at the
rasterizer stage.") because it's not really a size clamp as such but
rather a "poorman GL point rasterization implementation" but that's
really nitpicking.
(OT I'm actually also not sure why gl_Pointsize of 0.0 is deemed illegal
by OGL, I can't see why this would be problematic at least for
non-legacy rules.)