636002 - better messages about attrib index validation

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It would be nice to get some sort of warning in this case.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

You missed the 636000 mark. Now I think I'll just ignore your report.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

glGetAttribLocation on unused variables is a special case: it's not a GL error, it just returns the special value -1. I'm actually OK to generate a JS warning in this case since there's probably no good use case for that and it's confused many users in the past.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #2)
> glGetAttribLocation on unused variables is a special case: it's not a GL error,
> it just returns the special value -1. I'm actually OK to generate a JS warning
> in this case since there's probably no good use case for that and it's confused
> many users in the past.

Yes please. It confused me.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: patch

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 514507 [details] [diff] [review]
patch

no LogMessage, please -- this isn't an error (or even an unexpected thing); having the message in there will cause log spam for no reason when someone's running with verbose enabled trying to catch real errors.

(Not an error because you can use multiple shader programs with one set of getAttribLocation calls etc., automatically skipping/ignoring those that aren't used by the current program)

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(To add more detail:

let's say you have 3 sets of input attribs, say vertices, normals, colors, texcoords.  And then let's say you had three different shader programs for three different passes -- first one only uses, say, vertices, normals, and colors because it just draws a base diffuse layer.  Second one only uses vertices, normals, and texcoords, since it does some kind of lit texturing.  Third one only uses vertices and texcoords, since it does some kind of highlight rendering.

The way the -1 etc. bits are set up, you can do an identical pass of calls for each of these shader programs -- you get -1 back to say "this shader doesn't use this attrib", or in other words "this shader doesn't care about this".  Then you can skip binding it, thus keeping a single code flow without your code necessarily needing to know exactly what attrib streams each shader uses.  If in the future the second shader decides to use vertex colors, it all just magically works without any code changes.)

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

OK, I knew that wasn't an error, but I didn't know that there was a legitimate use case for that.

But still, in many cases, this is just confusing and printing a warning message would help.

Here's a counterproposal: when people inadvertently use this -1 value in other functions such as EnableVertexAttribArray, where it is an error, we could special case -1 and print a different warning message in that case: "this bogus value -1 probably comes from some getAttribLocation where it means that..."

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #7)
> Here's a counterproposal: when people inadvertently use this -1 value in other
> functions such as EnableVertexAttribArray, where it is an error, we could
> special case -1 and print a different warning message in that case: "this bogus
> value -1 probably comes from some getAttribLocation where it means that..."

I'd be happy with this approach too.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

sure, that's fine -- we already report an error in that case (though, for example, vertexAttribPointer takes an unsigned int -- so you'll have to check for uint32(-1) etc.)

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: better messages about attrib index validation

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/87e29a9b0096