Vertex attributes in GLSL

Compiling or Editing the Panda Source
#1

Hi,
I need tangent and binormal in my GLSL shaders, and they are not passed. The change to add them is quite simple, here is the diff:

Index: glShaderContext_src.cxx
===================================================================
RCS file: /cvsroot/panda3d/panda/src/glstuff/glShaderContext_src.cxx,v
retrieving revision 1.72
diff -u -r1.72 glShaderContext_src.cxx
--- glShaderContext_src.cxx	17 Nov 2011 19:42:51 -0000	1.72
+++ glShaderContext_src.cxx	25 Nov 2011 15:11:17 -0000
@@ -20,6 +20,7 @@
 #include "Cg/cgGL.h"
 #endif
 #include "pStatTimer.h"
+#include <algorithm>
 
 #define DEBUG_GL_SHADER 0
 
@@ -381,12 +382,30 @@
           }
           if (noprefix.substr(0, 13)  == "MultiTexCoord") {
             bind._name = InternalName::get_texcoord();
-            bind._append_uv = atoi(inputname->get_name().substr(13).c_str());
+            bind._append_uv = atoi(noprefix.substr(13).c_str());
             s->_var_spec.push_back(bind);
             gsg->_glBindAttribLocation(_glsl_program, i, param_name);
             continue;
           }
-          GLCAT.error() << "Unrecognized vertex attrib '" << param_name << "'!\n";
+          if (noprefix.substr(0, 7)  == "Tangent") {
+            bind._name = InternalName::get_tangent();
+            bind._append_uv = atoi(noprefix.substr(7).c_str());
+            s->_var_spec.push_back(bind);
+            gsg->_glBindAttribLocation(_glsl_program, i, param_name);
+            continue;
+          }
+          if (noprefix.substr(0, 8)  == "Binormal") {
+            bind._name = InternalName::get_binormal();
+            bind._append_uv = atoi(noprefix.substr(8).c_str());
+            s->_var_spec.push_back(bind);
+            gsg->_glBindAttribLocation(_glsl_program, i, param_name);
+            continue;
+          }
+          bind._name = InternalName::get_root();
+          std::replace(noprefix.begin(), noprefix.end(), '_', '.');
+          bind._name = bind._name->append(noprefix);
+          s->_var_spec.push_back(bind);
+          gsg->_glBindAttribLocation(_glsl_program, i, param_name);
           continue;
         }
       }

It also adds possibility of custom vertex attributes from .egg files as in this thread.

But it doesn’t work, because data from vertex arrays is not passed to OpenGL. This is because update_shader_vertex_arrays() (in glShaderContext_src.cxx) is not called when shader language is not Cg. See the 2503 line in glGraphicsStateGuardian_src.cxx.

  if (_current_shader_context == 0 || !_current_shader_context->uses_custom_vertex_arrays()) {
    // No shader, or a non-Cg shader.

Everything works after deleting ‘|| !_current_shader_context->uses_custom_vertex_arrays()’ part. ‘uses_custom_vertex_arrays()’ simply checks if shader is Cg. So is it ok to delete this?

Can someone look at this and commit these changes please?

Another strange behavior is that uniform matrices (like gl_ModelViewMatrix) are working with ‘gl_’ but do not with ‘p3d_’ prefix (glsl 3.3). I don’t know why. They seem to be passed to OpenGL in both cases. But this is less important, one can use ‘gl_’ in shaders.

#2

Thanks! I committed the p3d_Tangent and p3d_Binormal changes, with a minor change (p3d_Tangent/Binormal without a number is now also accepted). I changed uses_custom_vertex_arrays to return true for GLSL, too (I didn’t think it was necessary for GLSL when I wrote the GLSL code, I just didn’t think of binormals and tangents).

I’m not sure about your change of supporting other vertex arrays. Why are you replacing _ with .? It doesn’t seem obvious that it would work that way, what if someone had a vertex attribute named “vertex_offset” or so?

Instead, I changed it to bind every GLSL vertex attrib without a prefix to a Panda attrib of the same name. So you can bind to a Panda column name called “blah” by just declaring a varying vector in the GLSL shader called “blah”. Will this work for you?
I kept p3d_Tangent and p3d_Binormal in there, though.

#3

Thanks for committing :slight_smile:

I was replacing _ with . because this was done for Cg shaders. I don’t need this, and I agree that it makes more sense without replacing.

#4

I’m still a bit confused how to access vertex data in a GLSL shader. Is it possible, and how do I declare it in my shader?

For example I have some custom vertex data created with the entry named “mydata” in an EGG file and need to access it in a GLSL shader, equivalent to how I can use “in float4 vtx_mydata : TEXCOORD2” in a CG shader.

#5

teedee, I think with the current GLSL shader code it’s possible to access custom vertex attributes by just referencing them by name, i.e. if you use “vtx_mydata” in Cg you’d use simply “mydata” in GLSL.

I haven’t verified this though, I’m simply guessing from looking at the code. In the existing code from CVS, Cg and GLSL shaders run through completely different parsers, hence the differences with “gl_”, “p3d_”, “vtx_” etc.

I have a custom local build that runs everything through the same parser, so GLSL shaders can use the exact same “vtx_” and etc. syntax that Cg uses (which also helps the shader generator create working GLSL code), but I don’t know what to do with/about my changes. A topic for another day I suppose.

–jonah

#6

Hmmm, I tried “uniform vec4 mydata;” but there is just spam in the console about the shader input being missing.
“varying vec4 mydata;” results in the data being zeroes.

Would it be possible to get a patch of your changes to test out? Just the files in non-patch form would be great as well.

#7

My GLSL changes were part of my larger effort to build Android & iOS versions of Panda, including making usable GLES/ES2 pipes and lots of other general cleanup (see here: rococomedia.com/pub/changelog_20121208.txt).

It’d be possible to pull out a working patch, but it’d take quite a bit of effort to create it or even identify a valid minimal set of changed files. I suppose I could zip up my entire code directory if you’re interested in trying that? Do take a look at the changelog from the link above though; my modifications are fairly extensive and non-trivial.

–jonah

#8

Sure I’d be up for taking a look at your code and pulling the relevant changes out.

Of course if anyone knows a way to refer to vertex data in GLSL shaders in the current build that would be excellent too. :slight_smile:

#9

I sent you a PM with a link to an archive of my latest code.

I should also mention that if you can give me a non-functional example (data + shader), I’d be happy to take a look to figure out why the code in CVS isn’t working as expected.

–jonah

#10

I think you refer just by name, without any prefixes.
But in shader you specify “in vec4 mydata;” for OpenGl 3.x and 4.x (GLSL >= 1.3 I think) or “attribute vec4 mydata;” for OpenGL 2.x.

#11

Thanks for the info boorg, it has helped to uncover the problem.
I think the problem is that Panda is only looking at shader attributes if their name begins with “p3d_”.
If I put any vertex data name, correct or not, it will not generate any error and not have any effect, with the result being zeroed (black) data in the shader.
If, however, I put a “p3d_” prefix, I’ll get this error:

:display:gsg:glgsg(error): Unrecognized vertex attrib 'p3d_blend'!

I suspect that if I named my vertex data with a “p3d_” prefix on it then I would be able to access it in the shader.

#12

Uh…yeah, no. The relevant code in question is in glShaderContext_src.cxx (slightly edited for brevity):

if (inputname->get_name().substr(0, 3) == "gl_") {
  // We shouldn't bind anything to these.
} else if (inputname->get_name().substr(0, 4) == "p3d_") {
  noprefix = inputname->get_name().substr(4);
  if (noprefix == "Vertex") {
    bind._name = InternalName::get_vertex();
    ...
  }
  if (noprefix == "Normal") {
    bind._name = InternalName::get_normal();
    ...
  }
  if (noprefix == "Color") {
    bind._name = InternalName::get_color();
    ...
  }
  if (noprefix.substr(0, 7)  == "Tangent") {
    bind._name = InternalName::get_tangent();
    ...
  }
  if (noprefix.substr(0, 8)  == "Binormal") {
    bind._name = InternalName::get_binormal();
    ...
  }
  if (noprefix.substr(0, 13)  == "MultiTexCoord") {
    bind._name = InternalName::get_texcoord();
    ...
  }
  GLCAT.error() << "Unrecognized vertex attrib '" << param_name << "'!\n";
} else {
  bind._name = inputname;
  ...
}

By using the p3d_ prefix you’re getting into the ‘else if “p3d_”’ branch, which recognizes a fixed set of Panda vertex attributes. Your custom attribute isn’t one of them, so you fall through to the “Unrecognized vertex attrib” error. As I said originally, by not using either the “gl_” or “p3d” prefix you should end up in the final “else”, which will attempt to use the unadorned name verbatim.

The problem is probably not the name per se but Panda’s attempt to bind the data so that it’s supplied to the shader (in the function update_shader_vertex_arrays).

One easy possibility: what format is your extra data? The current CVS code has a hard time with anything other than 4 floats; specifically it doesn’t work with 4 bytes, e.g. as might be used for a color in a packed RGBA format.

Again I’d be willing to look more deeply into the problem if you want to give me broken example code + data.

–jonah

#13

Ah, I thought I had something. :frowning:
It was late and I didn’t investigate far enough.
The data are all 4-floats already.
I will prepare a simple sample model, shader, and script to demonstrate.

#14

Here is a self-contained example script:

from panda3d.core import NodePath, Shader, StringStream
from panda3d.egg import EggData, loadEggData
from direct.showbase.ShowBase import ShowBase

vshader = """//GLSL
#version 120
#extension GL_ARB_compatibility : enable

attribute vec4 blend;

void main() {
  gl_FrontColor = blend;
  gl_Position = gl_ModelViewProjectionMatrix * gl_Vertex;
}
"""

fshader = """//GLSL
#version 120
#extension GL_ARB_compatibility : enable

void main() {
  gl_FragColor = gl_Color;
}
"""

eggfile = """<CoordinateSystem> { Y-Up }

<Group> MDL-Cell_0_-1_world {
  <VertexPool> SHP-Cell_0_-1_world-ORG {
    <Vertex> 0 {
      5.13870421776 1.22309335316e-15 -22.3050122416
      <Aux> blend { 0.249930486083 0.864899754524 1.0 0.937254905701 }
      <RGBA> { 0.477431373835 0.477431373835 0.477431373835 0.749019622803 }
    }
    <Vertex> 1 {
      -13.0 0.0 -7.0
      <Aux> blend { 0.710710883141 0.748072206974 0.0 0.937254905701 }
      <RGBA> { 0.478431373835 0.478431373835 0.478431373835 0.749019622803 }
    }
    <Vertex> 2 {
      -11.5 0.0 -4.0
      <Aux> blend { 0.673182845116 0.823128163815 0.0 0.937254905701 }
      <RGBA> { 0.478431373835 0.478431373835 0.478431373835 0.749019622803 }
    }
    <Vertex> 3 {
      5.22291637975 -6.65052887007e-17 4.01522445801
      <Aux> blend { 0.254798233509 0.996485114098 1.0 0.937254905701 }
      <RGBA> { 0.478431373835 0.478431373835 0.478431373835 0.749019622803 }
    }
  }
  <Polygon> {
    <VertexRef> { 0 1 2 3 <Ref> { SHP-Cell_0_-1_world-ORG } }
  }
}
"""

class Game(ShowBase):
    def __init__(self):
        ShowBase.__init__(self)
        egg = EggData()
        egg.read(StringStream(eggfile))
        self.model = NodePath(loadEggData(egg))
        shader = Shader.make(Shader.SLGLSL, vshader, fshader)
        self.model.setShader(shader)
        self.model.reparentTo(self.render)

Game().run()
#15

Ugh, spent a couple hours staring at this only to discover I was looking in the wrong place. Turns out there’s two interacting issues:

  1. As we discussed a while back (here: [url]GLSL shader problem]), the use of the “gl_” prefix is problematic. In your shader I changed gl_Vertex to p3d_Vertex, and prepended the necessary “attribute vec4 p3d_Vertex;” declaration.

  2. The function uses_custom_vertex_arrays() is broken…again. Here’s a patch for a one-line fix:

--- panda/src/glstuff/glShaderContext_src.I 2012-11-13 07:28:35.000000000 -0800
+++ panda/src/glstuff/glShaderContext_src.I 2012-11-13 07:28:35.000000000 -0800
@@ -46,7 +46,7 @@
   #ifdef OPENGLES_2
     return true;
   #else
-    return (_shader->get_language() == Shader::SL_Cg);
+    return (_shader->get_language() != Shader::SL_none);
   #endif
 }

With those two modifications the extra vertex data was available as expected in the shader.

Hope that helps, let me know if you’re still having problems. I continue to be amazed that you’re planning on shipping a game with GLSL shaders, given the, uh, “functionally impaired” nature of the current implementation…

–jonah

GLSL -- models invisible in latest dev builds
#16

Much thanks, that seems to have done it!

I will just abandon my use of gl_ prefix. I can’t remember exactly why I was using it, probably because something else was broken at the time.

The only other issue is p3d_Color seems to be comically providing colors in the 0 to 255 range, whereas gl_Color gave the correct 0 to 1 range. This is easy enough to work around in the shader.

Thanks again for looking into this issue.

#17

I had reverted the “fix” because it broke the simplest of GLSL shaders - in fact, this “fix” is what broke gl_ in the first place. Again, both gl_ and p3d_ are supposed to work, and higher priority is placed on making gl_ work. I’ll be making a proper fix before the 1.8.1 release, though.

Thanks for pointing out the p3d_Color issue, teedee. I’ll look into that - I’m guessing that Panda should be passing them as integers instead of floats, so that OpenGL correctly maps them to the 0-1 range.

#18

rdb, I confess I’m a little put-off by your tone (something about the quotes around “fix”, I guess), so forgive me if I’m a bit rude myself.

  • I’m curious where the “higher priority placed on making gl_ work” comes from. Was there some discussion of this, either in these forums or elsewhere, or did you just arbitrarily declare that that’s the way you want it to be?

  • Putting a higher priority on the “gl_” prefix doesn’t make a lot of sense to me, given that almost all of the “gl_” variables have been deprecated since GLSL 1.3 circa 2008 (reference: http://goo.gl/jD2UD).

  • Without the “fix” “p3d_” variables don’t work. As discussed here and in several other threads, that breaks a lot of things.

  • There are people trying to use GLSL right now, today. For them, having something that mostly works (i.e. the “fix”) is probably a higher priority than supporting the deprecated “gl_” prefix and having the rest of GLSL work at some time in the future, when you have the time to fix it the way you prefer.

All of that said, as I mention above I’ve done a huge amount of rework on Panda’s support of GLSL to get all of this working in my local branch, making the issue largely an academic one for me personally. I was attempting to help out the various people who’re having trouble with the current CVS head and/or dev build, and offer an easy solution for today. Henceforth I think I’ll be more inclined to just bite my tongue.

Regardless, I look forward to seeing how you resolve this, and the many other problems with Panda’s current GLSL support, at some time in the future.

–jonah

#19

I did not mean to sound rude, I merely used quotes around the word “fix” because although it fixes the p3d inputs, it breaks gl inputs.

GLSL 1.3+ is feature of OpenGL 3 and above, which deprecates the fixed-function pipeline altogether (which is why the variables that access the FFP state are disappearing). We currently have no support for OpenGL 3, as Panda’s current system heavily relies on the fixed-function pipeline. We use regular old-fashioned state changes. If a driver happens to accept GLSL 1.3+ shaders in an OpenGL 2 context, that’s the driver’s prerogative; presumably, most recent drivers do, but I cannot make that assumption. So I do believe that it is imperative to support GLSL 1.2 shaders.

(In fact, when I added the p3d inputs, I wasn’t even keeping regular OpenGL in mind; it was to add support for GLES 2 SL, which has no gl_ inputs whatsoever. I never extensively tested the p3d stuff (hence the issue with p3d_Color), and as such I’m positively surprised that it works so well for people.)

I should add to that that the p3d inputs don’t even cover the entire spectrum of state inputs yet that gl_ covers, so I do think that supporting gl_ is imperative. Not to mention that gl_ inputs are handled by the driver and therefore work out of the box (up until 1.8.0, when I had stupidly introduced the fix without testing regular gl_ shaders).

Should this change? Yes, we should work toward overhauling our pipeline to be less FFP-oriented. Should we support new GLSL versions, and should both approaches work? Yes, they should. I was merely saying that I’m personally putting more focus on getting the existing stuff working properly before we start moving forward in supporting OpenGL 3 features, in general.

But I suppose that by reverting the fix I was picking one bug over another, which is arguably an arbitrary decision. But again, I will make sure that the next release of Panda will fix both bugs.

And with it, “gl_” variables don’t work. That also breaks many things, as I’ve received many complaints from people who use “old-fashioned” GLSL. But you’re right, p3d_ variables are supposed to work just as well, and again, it is the topmost item on my todo list to make both of these work.

I would again like to emphasise is that the way I prefer is that both gl_ and p3d_ inputs work perfectly.

I’m sorry to hear you say that, because we greatly appreciate the contributions you have provided and the issues that you have brought to our attention. I’m sorry that I have offended you by reverting your fix, but I promise that I’m working on a fix that will make both p3d_ and gl_ inputs work; it is my top priority for 1.8.1. But I’ve been very ill lately, which is why I have not been able to get around to it yet.

Nevertheless, I’m honestly grateful that you bring these things to attention; I think our community needs more people like you, and I hope that we’ll be able to enjoy your support for a while longer. :slight_smile:

#20

OK, I’ve finally checked in some fixes. I fixed various issues that I found, plus I’ve made it possible to use either gl_Vertex or p3d_Vertex, or even both at the same time (not recommended, but still possible). Alternatively, you can of course always refer to them by name, using simply ‘attribute vec4 vertex’.

This means that I’ve also put back the fix, so it is now once again totally possible to use p3d_ inputs if you wish. While I was at it, I’ve also added support for the “Inverse” and “Transpose” suffices to “p3d_ProjectionMatrix” and the like.

I also fixed the issue with the p3d_Color value range, this turned out to be as simple as changing a GL_FALSE to GL_TRUE somewhere.

Sorry that it took a while, and thanks again for helping out.