Need fixes

[1vanK]

1) GlDelegates.cs

#pragma warning disable 0649

2) IntPtr GetString(...) -> String GetString(...)

[1vanK]

Int32 GetAttribLocation(...) -> uint GetAttribLocation(...)

[giawa]

Hi 1vanK,

Thanks for the comments! I have committed an update that includes the pragma directive to get rid of the warnings.

Converting IntPtr to a String is a breaking change - however, I think it is worth it, so I have made that update. I am using the Marshal class to convert the pointer to a string safely. Please let me know what you think.

GetAttribLocation returns a GLint according to the OpenGL man pages. So I believe Int32 is actually correct. Can you please confirm this? https://www.opengl.org/sdk/docs/man/html/glGetAttribLocation.xhtml

Thanks,

Giawa

[1vanK]

I am currently experimenting with C # and Opengl and I have to do the conversion

    var positionLocation = Gl.GetAttribLocation(shaderProgram, "position");
    Gl.VertexAttribPointer((uint)positionLocation, 3, VertexAttribPointerType.Float, false, vertexSize, new IntPtr(vertexOffsetPosition));

I do not know what type is right %)

[giawa]

Unfortunately, that is correct. The man pages for VertexAttribPointer show that the type is GLuint whereas the man pages for GetAttribLocation show that the type is GLint. I don't know why they decided to go that route.

What we could do is create an overload for VertexAttribPointer that accepts an integer and does the casting internally. What do you think?

[1vanK]

Perhaps opengl follows the ideology of C which uses weak typing. But overload will be useful for a programmer in C#

[giawa]

Sounds good, I'll start taking a look over the weekend. You're also more than welcome to issue a pull request if you would like. Happy New Years!

[1vanK]

Happy New Years! :)

[1vanK]

Is GlLoad.cs unnecessary?

[giawa]

GlLoad.cs was superseded by GlReload.cs. I can remove GlLoad.cs to avoid confusion. Thanks!

[giawa]

The OpenGL man pages suggest that a negative value can be returned when called GetAttribLocation, which indicates an error. A negative value for an attribute it not allowed, and should be checked for before calling VertexAttribPointer. To do the 'right thing' I would need to create a VertexAttribPointer method that accepted a signed integer, and it would then need to check for a negative value first before casting to a uint, possibly duplicating user code. In that case, it is likely best to leave it up to the user code to perform this check and leave the current library code as-is.

What do you think? Would you prefer to see a version of the code that simply accepts a signed int type of VertexAttribPointer, effectively implementing weak typing? How can we solve this issue while staying true to both the OpenGL man pages and to C#'s type system.

[1vanK]

I think we need just throw exception for negative argument, it is C# way :)

[WardBenjamin]

Agreed. Exceptions for negative arguments also encourages practices for negative number checking similar to what would be used for other languages, so it's familiar and conformant to the man pages.

[giawa]

Ok, so the suggestion is to do something like this:

public static void VertexAttribPointer(Int32 index, Int32 size, OpenGL.VertexAttribPointerType type, Boolean normalized, Int32 stride, IntPtr pointer)
{
    if (index < 0) throw new ArgumentOutOfRangeException("index");
    Delegates.glVertexAttribPointer((UInt32)index, size, type, normalized, stride, pointer);
}

Then the code example from 1vanK simplifies to:

var positionLocation = Gl.GetAttribLocation(shaderProgram, "position");
Gl.VertexAttribPointer(positionLocation, 3, VertexAttribPointerType.Float, false, vertexSize, new IntPtr(vertexOffsetPosition));

which avoids the cast. Look ok?

Thanks for the comments!

Giawa

[1vanK]

Excellent!

[giawa]

Check out https://github.com/giawa/opengl4csharp/commit/0ad212dc12d98eea6767d87b305d886b4e03f174 to see if this does the trick. Let me know if you notice any methods that should not (or should have) gotten this treatment. Feel free to re-open this issue. Thanks!