[Cogl] [PATCH] attribute: Adds support for constant CoglAttributes

[Robert Bragg]

On Fri, Sep 28, 2012 at 5:08 PM, Neil Roberts <neil at linux.intel.com> wrote:
> Here are some comments inline:
>
> Robert Bragg <robert at sixbynine.org> writes:
>
>> +static CoglAttribute *
>> +_cogl_attribute_new_const (CoglContext *context,
>> +                           const char *name,
>> +                           int n_components,
>> +                           int n_columns,
>> +                           CoglBool transpose,
>> +                           const float *value)
>
> It seems like this should use the default value for whether the
> attribute is normalized and also verify that the number of components
> makes sense in the same way that cogl_attribute_new does.

Yeah, I added the validation as you suggest, though the normalized
state can be ignored given that we only support floating point const
attributes currently.

>
>> +                                    2, /* 1 column vector */
>
> This comment and the other similar ones look like a cut-and-paste error.

Oops, yeah I've fixed that now.

>
>>  static void
>>  _cogl_attribute_free (CoglAttribute *attribute)
>>  {
>> -  cogl_object_unref (attribute->attribute_buffer);
>> +  cogl_object_unref (attribute->d.buffered.attribute_buffer);
>
> Shouldn't this only free the buffer if the attribute isn't constant? If
> the attribute is constant presumably it needs to destroy the boxed value
> too, although maybe that doesn't really matter because all of the
> possible boxed values don't have any extra allocation anyway.

Ah, yep, I've fixed it to check before unrefing the buffer and destroy
the boxed value for good measure.

>
>> + * The constant @value is a single precision floating point scalar
>> + * which should have a corresponding declaration in GLSL code like:
>> + *
>> + * [|
>> + * varying float name;
>> + * |]
>
> The corresponding variable in GLSL would be an attribute not a varying.

right, I've fixed this thinko

>
> I wonder if we need some way to be able to modify the constant value. It
> would be kind of a shame if an application was repeatedly drawing with
> different values and they had to recreate the attribute every time
> because it would repeatedly have to revalidate the attribute name. I
> guess we could always add some extra API for that later though so maybe
> it's not worth worrying about for now.

My initial feeling is to stick with CoglAttribute being mostly
immutable (although technically you can set a new buffer on an
attribute currently we do at least warn if you do that mid-scene). By
keeping the attributes immutable that allows us to log them into a
deferred rendering command queue, which wouldn't work if applications
changed the constants repeatedly mid-scene.

I think though, as you say, this can be thought about separately.

I've pushed the patch with the fixes you suggested, thanks.

regards,
- Robert

>
> Regards,
> - Neil
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl