One more question:
for patch 2 of the series, there was the request to change the type of
_HasAttachments from GLboolean to bool. If the helper functions "survive",
should the helper functions return unsigned int instead of GLuint?
-Kevin
-----Original Message-----
From: Rogovin, Kevin
Sent: Wednesday, May 06, 2015 10:28 AM
To: 'Ian Romanick'; mesa-...@freedesktop.org
Subject: RE: [Mesa-dev] [PATCH 4/9] mesa: add helper convenience functions for
fetching geometry of gl_framebuffer
> I'm waffling about this a bit. I'm concerned that people will use
> buffer->Width when they should use the accessor. I don't see any
> changes to core Mesa code to use the accessor, so I'm a little concerned that
> some subtle, incorrect behavior is introduced... but there may well not be.
> A common idiom in Mesa is to have an _Value field that is the derived value.
> In many structures in mtypes.h you can see things like Enabled and
> _ReallyEnabled. One is what the API sets, and the other is what the driver
> uses that is based on the API setting and "other factors." In many ways, the
> existing gl_framebuffer Width and Height fields are already derived values
> based on the sizes of the attachments.
>For at least Width, Height, and MaxNumLayers, it seems better to set the
>existing fields differently when _HasAttachments is true. That reduces the
>number of places where a person has to think about _HasAttachments
>considerably... though perhaps there is something else that I'm not thinking
>of?
I was thinking of doing this when I was making the patch. The problem is that
Width, Height and MaxNumLayers were used for two different purposes:
1. Specifying the intersection of the attached buffers (this affects blitting
for example) 2. Specifying the "geometry" to send to a rasterizer
Changing Width and Height to be the geometry made me itch because Width and
Height are zero when there are no buffer attachments. I added a mess of
comments about the meaning of Width and Height. My concern is that other parts
of code use Width and Height being 0 to do other things. With this patch way,
the meaning of Width and Height has not changed; instead a diver has to "know"
what it is doing when it enables the extension.
I am not particular about this though; I made it this way in an attempt to
reduce the scope of changes from this patch series. If people want that the
meaning of Width, Height and MaxSamplers changes I can go with that too, I just
think it can increase the ickiness of making sure no unintended side effects
are added is ickier.
> If we go that route, we should probably just add a gl_framebuffer::Samples
> field for uniformity.
> +
> +
> +static inline GLuint
> +_mesa_geometric_height(const struct gl_framebuffer *buffer) {
> + return buffer->_HasAttachments ?
> + buffer->Height : buffer->DefaultGeometry.Height; }
> +
> +static inline GLuint
> +_mesa_geometric_samples(const struct gl_framebuffer *buffer) {
> + return buffer->_HasAttachments ?
> + buffer->Visual.samples : buffer->DefaultGeometry.NumSamples;
> +}
> +
> +static inline GLuint
> +_mesa_geometric_layers(const struct gl_framebuffer *buffer) {
> + return buffer->_HasAttachments ?
> + buffer->MaxNumLayers : buffer->DefaultGeometry.Layers; }
> +
> extern void
> _mesa_update_draw_buffer_bounds(struct gl_context *ctx);
>
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h index
> ef97538..f0e8fbc 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -3178,7 +3178,13 @@ struct gl_framebuffer
> * the values in DefaultGeometry to initialize its
> * viewport, scissor and so on (in particular _Xmin,
> * _Xmax, _Ymin and _Ymax do NOT take into account
> - * _HasAttachments being false)
> + * _HasAttachments being false). To get the geometry
> + * of the framebuffer, the helper functions
> + * _mesa_geometric_width(),
> + * _mesa_geometric_height(),
> + * _mesa_geometric_samples(),
> + * _mesa_geometric_layers()
> + * are available that check _HasAttachments.
> */
> GLboolean _HasAttachments;
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
