[Mesa-dev] [PATCH] vbo: Correctly handle attribute offsets in dlist draw.

[Mathias Fröhlich]

Hi,

On Friday, 19 August 2016 19:47:13 CEST Timothy Arceri wrote:
> On Fri, 2016-08-19 at 10:52 +0200, Erik Faye-Lund wrote:
> > On Fri, Aug 19, 2016 at 10:38 AM, Mathias Fröhlich
> > <Mathias.Froehlich at gmx.net> wrote:
> > > 
> > > On Friday, 19 August 2016 09:20:41 CEST Erik Faye-Lund wrote:
> > > > 
> > > > On Thu, Aug 18, 2016 at 7:50 PM,  <Mathias.Froehlich at gmx.net>
> > > > wrote:
> > > > > 
> > > > > From: Mathias Fröhlich <mathias.froehlich at web.de>
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > I found the below while fixing a similar problem lately in
> > > > > the immediate mode glBegin/glEnd code path.
> > > > > 
> > > > > Please review
> > > > > Thanks
> > > > > 
> > > > > Mathias
> > > > > 
> > > > > 
> > > > > 
> > > > > When executing a display list draw with a shader program
> > > > > using the generic0 attribute the offset computation
> > > > > may get out of sync. To fix precompute the offsets
> > > > > on the full attribute list and store the offsets in
> > > > > the display list node.
> > > > > 
> > > > > Signed-off-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> > > > > ---
> > > > >  src/mesa/vbo/vbo_save.h      |  1 +
> > > > >  src/mesa/vbo/vbo_save_api.c  |  6 ++++++
> > > > >  src/mesa/vbo/vbo_save_draw.c | 35 +++++++++++++++++-----------
> > > > > -------
> > > > >  3 files changed, 24 insertions(+), 18 deletions(-)
> > > > > 
> > > > > diff --git a/src/mesa/vbo/vbo_save.h b/src/mesa/vbo/vbo_save.h
> > > > > index 2843b3c..a61973f 100644
> > > > > --- a/src/mesa/vbo/vbo_save.h
> > > > > +++ b/src/mesa/vbo/vbo_save.h
> > > > > @@ -64,6 +64,7 @@ struct vbo_save_vertex_list {
> > > > >     GLbitfield64 enabled; /**< mask of enabled vbo arrays. */
> > > > >     GLubyte attrsz[VBO_ATTRIB_MAX];
> > > > >     GLenum attrtype[VBO_ATTRIB_MAX];
> > > > > +   GLushort offsets[VBO_ATTRIB_MAX];
> > > > >     GLuint vertex_size;  /**< size in GLfloats */
> > > > > 
> > > > >     /* Copy of the final vertex from node->vertex_store-
> > > > > >bufferobj.
> > > > > diff --git a/src/mesa/vbo/vbo_save_api.c
> > > > > b/src/mesa/vbo/vbo_save_api.c
> > > > > index f648ccc..4473fef 100644
> > > > > --- a/src/mesa/vbo/vbo_save_api.c
> > > > > +++ b/src/mesa/vbo/vbo_save_api.c
> > > > > @@ -436,6 +436,12 @@ _save_compile_vertex_list(struct
> > > > > gl_context *ctx)
> > > > >     node->vertex_size = save->vertex_size;
> > > > >     node->buffer_offset =
> > > > >        (save->buffer - save->vertex_store->buffer) *
> > > > > sizeof(GLfloat);
> > > > > +   GLushort offset = 0;
> > > > > +   int i;
> > > > > +   for (i = 0; i < VBO_ATTRIB_MAX; ++i) {
> 
> You can just make this:
> 
>    GLushort offset = 0;
>    for (int i = 0; i < VBO_ATTRIB_MAX; ++i) {
> 
> Which is neater IMO.
Indeed, Changed locally.

> 
> > > > > +      node->offsets[i] = offset;
> > > > > +      offset += node->attrsz[i] * sizeof(GLfloat);
> > > > > +   }
> > > > 
> > > > I'm not sure what the policy regarding C99 is ATM, but this seems
> > > > to
> > > > introduce declaration-after-statement for the first time in this
> > > > source.
> > > 
> > > Someone requested lately in a review that I should declare
> > > variables where I
> > > use them the first time. To my following question, if this is now a
> > > treewide
> > > policy, I got no answer. IIRC the file in question was some mesa
> > > core file.
> > > I can easily change my personal coding back to 'adapt to the
> > > surronding code
> > > style' what I usually do for patches in any other project I
> > > contribute to and
> > > what I did on mesa before I got the request to start moving to
> > > somewhere else.
> > > Any official policy?
> > 
> > AFAIK, there used to be some parts of mesa that had to be compiled
> > with an older MSVC version without support for such C99-isms in
> > C-sources. But I *think* that requirement has been lifted still.
> > 
> > Personally, I'd just stay with the local style in the source, but I'm
> > no authority on this.
> 
> I'm pretty sure most devs danced a little jig when this restriction was
> lifted [1]. I believe the preference is to declare variables where they
> are used rather than sticking to the old style.
Me too.

> [1] https://lists.freedesktop.org/archives/mesa-dev/2016-February/10734
> 7.html
Great!! Thanks for the reference!!

best
Mathias