On 4 November 2011 16:43, Marek Olšák <[email protected]> wrote: > Hi Paul, > > I won't comment on the patch 1, because I didn't write it, I was just > preserving history. > > On Fri, Nov 4, 2011 at 10:00 PM, Paul Berry <[email protected]> > wrote: > [snip] > >> +bool parse_tfeedback_decl(const void *mem_ctx, const char *input, > >> + struct tfeedback_decl *decl) > >> +{ > >> + /* We don't have to be pedantic about what is a valid GLSL variable > >> name, > >> + * because any variable with an invalid name can't exist in the IR > >> anyway. > >> + */ > >> + > >> + const char *bracket = strrchr(input, '['); > >> + > >> + if (bracket) { > >> + decl->name = ralloc_strndup(mem_ctx, input, bracket - input); > >> + if (sscanf(bracket, "[%u]", &decl->array_index) == 1) { > >> + decl->is_array = true; > >> + return true; /* Found: "var[i]" */ > >> + } > >> + } else { > >> + decl->name = ralloc_strdup(mem_ctx, input); > >> + return true; > >> + } > >> + > >> + return false; > >> +} > > > > For non-arrays, this function doesn't assign to decl->is_array or > > decl->array_index. That's a problem, because its caller > > (ir_set_transform_feedback_outs::ir_set_transform_feedback_outs()) passes > > uninitialized memory for decl. We could fix the problem by having > > ir_set_transform_feedback_outs::ir_set_transform_feedback_outs() > initialize > > the memory first, but I would prefer if we fixed the problem by always > > assigning to all fields of decl, since we're effectively treating it as > an > > output of this function. > > Yeah, that was unintentional. > > Regarding the other comments, I don't know the GLSL compiler so well. > I had forked ir_set_program_inouts and was basing upon that. Now that > I think about it, it makes more sense to do it like you say. :) > > Concerning this: > > varying mat4 r; > glTransformFeedbackVaryingsEXT(.. "r[1]" ..); > > The EXT spec doesn't state whether this is legal. It says nothing > about matrices, actually. > > [snip] > > > > The patch series doesn't add any code that calls this function. Is that > > planned for a future patch? > > > > The function is already used here: > http://cgit.freedesktop.org/~mareko/mesa/log/?h=transform-feedback > > It's a work in progress, but EXT_transform_feedback already works > quite well and ARB_transform_feedback2 too except > glDrawTransformFeedback, which isn't fully implemented yet. Despite > your remarks, the GLSL linker works pretty good for me, so I am going > to take a break from it for a while. > > Feel free to take over all 3 patches and implement it properly, as I > might not have time to get to it again soon. >
Ok, I've worked up an alternative proposal, which I'll send out shortly.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
