Chapter2/6-MultipleLights sample is broken

[lixiss]

Chapter2/6-MultipleLights sample are crash for me (I'm run it on Intel HD 4600 video, if it matters at all, doesn't sure, i'm completely newbie in OpenGL).

The issue is what Shader.cs fills _uniformLocations by using call GL.GetProgram(Handle, GetProgramParameterName.ActiveUniforms, out var numberOfUniforms);. However this call doesn't report all locations.

I'm checked LearnOpenGL example, and it uses bit different way, so i'm put few changes:

diff --git a/Common/Shader.cs b/Common/Shader.cs
index 1ff74d2..3ec0c16 100644
--- a/Common/Shader.cs
+++ b/Common/Shader.cs
@@ -155,8 +155,9 @@ namespace LearnOpenTK.Common
         /// <param name="data">The data to set</param>
         public void SetFloat(string name, float data)
         {
+            var uniformLocation = GL.GetUniformLocation(Handle, name);
             GL.UseProgram(Handle);
-            GL.Uniform1(_uniformLocations[name], data);
+            GL.Uniform1(uniformLocation, data);
         }
 
         /// <summary>
@@ -182,8 +183,9 @@ namespace LearnOpenTK.Common
         /// <param name="data">The data to set</param>
         public void SetVector3(string name, Vector3 data)
         {
+            var uniformLocation = GL.GetUniformLocation(Handle, name);
             GL.UseProgram(Handle);
-            GL.Uniform3(_uniformLocations[name], data);
+            GL.Uniform3(uniformLocation, data);
         }
     }
 }

And that's makes example to run. Surely, this requires more accurate fix.

[NogginBops]

Hi, thanks for the report.

Could you please provide more information on what the error you get is?
What uniform locations are missing?

[lixiss]

Surely:

Shader: Shaders/shader.vert+Shaders/lighting.frag Handle: 3
Adding location: model => 0
Adding location: view => 1
Adding location: projection => 2
Adding location: material.diffuse => 3
Adding location: material.specular => 4
Adding location: material.shininess => 5
Adding location: viewPos => 6
Shader: Shaders/shader.vert+Shaders/shader.frag Handle: 4
Adding location: model => 0
Adding location: view => 1
Adding location: projection => 2
Unknown location: dirLight.direction => 7
Unknown location: dirLight.ambient => 8
Unknown location: dirLight.diffuse => 9
Unknown location: dirLight.specular => 10
Unknown location: pointLights[0].position => 11
Unknown location: pointLights[0].ambient => 27
Unknown location: pointLights[0].diffuse => 31
Unknown location: pointLights[0].specular => 35
Unknown location: pointLights[0].constant => 15
Unknown location: pointLights[0].linear => 19
Unknown location: pointLights[0].quadratic => 23
Unknown location: pointLights[1].position => 12
Unknown location: pointLights[1].ambient => 28
Unknown location: pointLights[1].diffuse => 32
Unknown location: pointLights[1].specular => 36
Unknown location: pointLights[1].constant => 16
Unknown location: pointLights[1].linear => 20
Unknown location: pointLights[1].quadratic => 24
Unknown location: pointLights[2].position => 13
Unknown location: pointLights[2].ambient => 29
Unknown location: pointLights[2].diffuse => 33
Unknown location: pointLights[2].specular => 37
Unknown location: pointLights[2].constant => 17
Unknown location: pointLights[2].linear => 21
Unknown location: pointLights[2].quadratic => 25
Unknown location: pointLights[3].position => 14
Unknown location: pointLights[3].ambient => 30
Unknown location: pointLights[3].diffuse => 34
Unknown location: pointLights[3].specular => 38
Unknown location: pointLights[3].constant => 18
Unknown location: pointLights[3].linear => 22
Unknown location: pointLights[3].quadratic => 26
Unknown location: spotLight.position => 39
Unknown location: spotLight.direction => 40
Unknown location: spotLight.ambient => 43
Unknown location: spotLight.diffuse => 44
Unknown location: spotLight.specular => 45
Unknown location: spotLight.constant => 46
Unknown location: spotLight.linear => 47
Unknown location: spotLight.quadratic => 48
Unknown location: spotLight.cutOff => 41
Unknown location: spotLight.outerCutOff => 42

Got this log with next code:

diff --git a/Common/Shader.cs b/Common/Shader.cs
index 1ff74d2..8ec8596 100644
--- a/Common/Shader.cs
+++ b/Common/Shader.cs
@@ -68,6 +68,7 @@ namespace LearnOpenTK.Common
 
             // First, we have to get the number of active uniforms in the shader.
             GL.GetProgram(Handle, GetProgramParameterName.ActiveUniforms, out var numberOfUniforms);
+            Console.WriteLine("Shader: {0}+{1} Handle: {2}", vertPath, fragPath, Handle);
 
             // Next, allocate the dictionary to hold the locations.
             _uniformLocations = new Dictionary<string, int>();
@@ -83,6 +84,8 @@ namespace LearnOpenTK.Common
 
                 // and then add it to the dictionary.
                 _uniformLocations.Add(key, location);
+
+                Console.WriteLine("Adding location: {0} => {1}", key, location);
             }
         }
 
@@ -155,8 +158,15 @@ namespace LearnOpenTK.Common
         /// <param name="data">The data to set</param>
         public void SetFloat(string name, float data)
         {
+            if (!_uniformLocations.TryGetValue(name, out var location))
+            {
+                location = GL.GetUniformLocation(Handle, name);
+                Console.WriteLine("Unknown location: {0} => {1}", name, location);
+                _uniformLocations.Add(name, location);
+            }
+
             GL.UseProgram(Handle);
-            GL.Uniform1(_uniformLocations[name], data);
+            GL.Uniform1(location, data);
         }
 
         /// <summary>
@@ -182,8 +192,15 @@ namespace LearnOpenTK.Common
         /// <param name="data">The data to set</param>
         public void SetVector3(string name, Vector3 data)
         {
+            if (!_uniformLocations.TryGetValue(name, out var location))
+            {
+                location = GL.GetUniformLocation(Handle, name);
+                Console.WriteLine("Unknown location: {0} => {1}", name, location);
+                _uniformLocations.Add(name, location);
+            }
+
             GL.UseProgram(Handle);
-            GL.Uniform3(_uniformLocations[name], data);
+            GL.Uniform3(location, data);
         }
     }
 }

PS: Sorry, i'm did not wrote initially, issue was in that _uniformLocations[name] evenutally throw exception, so i'm start learn OpenGL from creating issue for you. :)

[NogginBops]

Can you give me the exception message you get on _uniformLocations[name]?

[lixiss]

Can you give me the exception message you get on _uniformLocations[name]?

System.Collections.Generic.KeyNotFoundException: 'The given key 'dirLight.direction' was not present in the dictionary.'

[NogginBops]

Are you sure you are compiling the correct shaders?

[lixiss]

Shader files are unmodified, as they come from this repository.

[lixiss]

I'm did not have luck to find any reasonable answer why only seven or so entries returned as active uniforms... (for me).

Generally by this issue i'm point what LearnOpenGL did not use program introspection (is it exists some reason for this?). And set values by something like:

    void setFloat(const std::string &name, float value) const
    { 
        glUniform1f(glGetUniformLocation(ID, name.c_str()), value); 
    }

This way work, and as i'm already wrote, if Shader.cs turn to act in similar way - then sample start to work, and everything looks fine.

There is probably driver-specific behavior, however, glGetActiveUniform explicitly state how it deal with arrays: Only one active uniform variable will be reported for a uniform array., which indirectly means what each array element will never be tracked. This turn me to make implication what current code might be broken regardless to mine observed local behavior. [This is especially common for any kind of compiler, so nothing bad/new or non-logical here.]

[NogginBops]

The code runs on my GPU (nvidia gtx 1070).
This sounds like driver dependent behavior related to an edge case in program introspection to me.
It definitely could be the introspection that relies on non-spec behavior of drivers.
I would have to look into it a little bit further to know if that is the case.

[deccer]

Ah it seems there is also a typo.

if (!_uniformLocations.TryGetValue(name, out var location))
{
    location = GL.GetUniformLocation(Handle, name);

You invert the .TryGetValue by !. TryGetValue returns true if the value can be pulled out.

That is the problem ;)

code should look like this instead

public void SetVector3(string name, Vector3 data)
{
    if (_uniformLocations.TryGetValue(name, out var location))
    {
        GL.UseProgram(Handle); // this should be moved out actually, there is no need to call this for every parameter setting
        GL.Uniform3(location, data);
        return;
    }

    location = GL.GetUniformLocation(Handle, name);
    if (location == -1)
    {
        Console.WriteLine($"Uniform {name} cannot be found.");
        return;
    }
    _uniformLocations.Add(name, location);
}

But also you should move the location lookup to after linking the program, then you dont have to repeat this code over and over and only lookup, when found use, when not found throw error

[lixiss]

@deccer is not a typo. If TryGetValue get value from dictionary - then code block skipped, nothing logged and go as it was designed. If not pulled, then location value obtained and logged to screen and added to dictionary (mainly to log location value once). If you read log you should notice what all calls succeed. [And more over - renderered as it should]

However, thanks for your good intent.

PS:

code should look like this instead

So, definitely you reverse branches of my code effectively doubling number of lines without changing logic itself, except what forward/bottom branch no more call GL.Uniform3, which effectively bug (additionally, which will be masked by next render call). So, your version nor add readability or maintability, may be some kind of yours taste, but, nothing more. Using !TryGetValue is pretty standard practice, when you need value and put "value generator" just here (inside branch), and then use this value (regardless to from it come).

But also you should move the location lookup to after linking the program, then you dont have to repeat this code over and over and only lookup, when found use, when not found throw error

Again, thanks. It is clear for me from start. I guess samples don't do this just for simplicity. In my taste - if Shader.cs turn to it's C++ counterpart way - nothing will be lost (no need dictionary at all, performance is a question surely, but dictionary lookup is also bit naive in this sense), there is learning sample in first place.

[deccer]

Your "log" output clearly shows valid uniform locations, which it should not print at all, if the .TryGet was correct - which in your case was not. the ! needs to go.

We have a discord server if you want to hop on... https://discord.gg/6HqD48s

[lixiss]

@deccer my log clearly shows, what introspection don't return all uniforms in first place, so _uniformLocations never was filled completely as initial code expect. Everything rest really no matter, and code was born to answer question as comment above (which uniform locations was not returned by get active uniforms call?). Even dictionary manipulation was added here with intent to report uniform name only once, and not to trying memoizing GetUniformLocation result.

If you be more careful, you may find what this code is equivalent to your code, except, what this code set uniform value immediately, while your miss this step on second branch (I believe just as typo).

         public void SetFloat(string name, float data)
         {
            if (!_uniformLocations.TryGetValue(name, out var location))
            {
                // No variable in dictionary
                // Get variable location
                location = GL.GetUniformLocation(Handle, name);
                // Report
                Console.WriteLine("Unknown location: {0} => {1}", name, location);
                // Add this to dictionary, so in next time TryGetValue will succeed.
                _uniformLocations.Add(name, location);
            }

            // Here we always have correct location
            GL.UseProgram(Handle);
            GL.Uniform1(location, data);
         }

Anyway:

1. I'm already got sensible answer. (What it is more likely to driver issue.)
2. Point what this sample doesn't always work, and what "original" LearnOpenGL sample doesn't use introspection and original sample just work (as compared to today's state).
3. Learn what it is better to not rely on introspection whenever is possible.

So, my contribution ends here, and there is no need more discussion about this topic.

[deccer]

No no, I think I am stupid. The console output irritated me for some reason. You are right. :S I apologise.

I wonder what GPU you use and if the drivers are up to date.

[lixiss]

@deccer all is fine. :)

i'm wrote at initial message, it is Intel HD 4600 (i7-4770). I'm run Windows 10 stable with latest updates (windows care about intel drivers updates too in this case also automatically). I'm know what it is not very ideal GPU / software, but still it can complete many things easily (including this samples, even some good past games like DAO). It is just mine bad luck what when i'm pick samples - almost any work, but hit in this issue. (BTW, samples did not show exceptions, just silently close window on exception, so it can be discovered only with running by debugger.)