Provide typed OpenGL objects rather than raw uint32s

[myaaaaaaaaa]

The OpenGL XML spec provides type annotations that we can use to create proper Go types, which allows the compiler to catch mistakes such as swapped or incorrect parameters:

gl.AttachShader(shader, program)

The following sed script can be used to automatically rewrite some gl.Gen*() declarations:

sed -z -i -E 's/(var \w+) uint32(\s+gl.Gen)(\w+)(s\(1, &\w+\))/\1 gl.\3\2\3\4/g' *.go

-	var texture uint32
+	var texture gl.Texture
 	gl.GenTextures(1, &texture)

Click here to see a preview of what users of go-gl would need to do to port their code:

diff --git a/gl21-cube/cube.go b/gl21-cube/cube.go
index 4416ee9..9b00855 100644
--- a/gl21-cube/cube.go
+++ b/gl21-cube/cube.go
@@ -19,7 +19,7 @@ import (
 )
 
 var (
-	texture   uint32
+	texture   gl.Texture
 	rotationX float32
 	rotationY float32
 )
@@ -61,7 +61,7 @@ func main() {
 	}
 }
 
-func newTexture(file string) uint32 {
+func newTexture(file string) gl.Texture {
 	imgFile, err := os.Open(file)
 	if err != nil {
 		log.Fatalf("texture %q not found on disk: %v\n", file, err)
@@ -77,8 +77,8 @@ func newTexture(file string) uint32 {
 	}
 	draw.Draw(rgba, rgba.Bounds(), img, image.Point{0, 0}, draw.Src)
 
-	var texture uint32
 	gl.Enable(gl.TEXTURE_2D)
+	var texture gl.Texture
 	gl.GenTextures(1, &texture)
 	gl.BindTexture(gl.TEXTURE_2D, texture)
 	gl.TexParameteri(gl.TEXTURE_2D, gl.TEXTURE_MIN_FILTER, gl.LINEAR)
diff --git a/gl41core-cube/cube.go b/gl41core-cube/cube.go
index 0f32200..94e79fe 100644
--- a/gl41core-cube/cube.go
+++ b/gl41core-cube/cube.go
@@ -86,11 +86,11 @@ func main() {
 	}
 
 	// Configure the vertex data
-	var vao uint32
+	var vao gl.VertexArray
 	gl.GenVertexArrays(1, &vao)
 	gl.BindVertexArray(vao)
 
-	var vbo uint32
+	var vbo gl.Buffer
 	gl.GenBuffers(1, &vbo)
 	gl.BindBuffer(gl.ARRAY_BUFFER, vbo)
 	gl.BufferData(gl.ARRAY_BUFFER, len(cubeVertices)*4, gl.Ptr(cubeVertices), gl.STATIC_DRAW)
@@ -139,7 +139,7 @@ func main() {
 	}
 }
 
-func newProgram(vertexShaderSource, fragmentShaderSource string) (uint32, error) {
+func newProgram(vertexShaderSource, fragmentShaderSource string) (gl.Program, error) {
 	vertexShader, err := compileShader(vertexShaderSource, gl.VERTEX_SHADER)
 	if err != nil {
 		return 0, err
@@ -174,7 +174,7 @@ func newProgram(vertexShaderSource, fragmentShaderSource string) (uint32, error)
 	return program, nil
 }
 
-func compileShader(source string, shaderType uint32) (uint32, error) {
+func compileShader(source string, shaderType uint32) (gl.Shader, error) {
 	shader := gl.CreateShader(shaderType)
 
 	csources, free := gl.Strs(source)
@@ -197,7 +197,7 @@ func compileShader(source string, shaderType uint32) (uint32, error) {
 	return shader, nil
 }
 
-func newTexture(file string) (uint32, error) {
+func newTexture(file string) (gl.Texture, error) {
 	imgFile, err := os.Open(file)
 	if err != nil {
 		return 0, fmt.Errorf("texture %q not found on disk: %v", file, err)
@@ -213,7 +213,7 @@ func newTexture(file string) (uint32, error) {
 	}
 	draw.Draw(rgba, rgba.Bounds(), img, image.Point{0, 0}, draw.Src)
 
-	var texture uint32
+	var texture gl.Texture
 	gl.GenTextures(1, &texture)
 	gl.ActiveTexture(gl.TEXTURE0)
 	gl.BindTexture(gl.TEXTURE_2D, texture)

Click here to see a preview of the go-gl/gl diff (truncated for brevity):

diff --git a/v4.1-core/gl/package.go b/v4.1-core/gl/package.go
index 0b58b86..5bee367 100644
--- a/v4.1-core/gl/package.go
+++ b/v4.1-core/gl/package.go
@@ -5277,6 +5277,20 @@ import (
 	"unsafe"
 )
 
+type (
+	Buffer            uint32
+	Framebuffer       uint32
+	Program           uint32
+	ProgramPipeline   uint32
+	Query             uint32
+	Renderbuffer      uint32
+	Sampler           uint32
+	Shader            uint32
+	Texture           uint32
+	TransformFeedback uint32
+	VertexArray       uint32
+)
+
 const (
 	ACCUM_ADJACENT_PAIRS_NV                                    = 0x90AD
 	ACTIVE_ATOMIC_COUNTER_BUFFERS                              = 0x92D9
@@ -8707,15 +8721,15 @@ func boolToInt(b bool) int {
 	}
 	return 0
 }
-func ActiveProgramEXT(program uint32) {
+func ActiveProgramEXT(program Program) {
 	C.glowActiveProgramEXT(gpActiveProgramEXT, (C.GLuint)(program))
 }
 
 // set the active program object for a program pipeline object
-func ActiveShaderProgram(pipeline uint32, program uint32) {
+func ActiveShaderProgram(pipeline ProgramPipeline, program Program) {
 	C.glowActiveShaderProgram(gpActiveShaderProgram, (C.GLuint)(pipeline), (C.GLuint)(program))
 }
-func ActiveShaderProgramEXT(pipeline uint32, program uint32) {
+func ActiveShaderProgramEXT(pipeline ProgramPipeline, program Program) {
 	C.glowActiveShaderProgramEXT(gpActiveShaderProgramEXT, (C.GLuint)(pipeline), (C.GLuint)(program))
 }
 
@@ -8728,7 +8742,7 @@ func ApplyFramebufferAttachmentCMAAINTEL() {
 }
 
 // Attaches a shader object to a program object
-func AttachShader(program uint32, shader uint32) {
+func AttachShader(program Program, shader Shader) {
 	C.glowAttachShader(gpAttachShader, (C.GLuint)(program), (C.GLuint)(shader))
 }
 
@@ -8747,10 +8761,10 @@ func BeginPerfQueryINTEL(queryHandle uint32) {
 }
 
 // delimit the boundaries of a query object
-func BeginQuery(target uint32, id uint32) {
+func BeginQuery(target uint32, id Query) {
 	C.glowBeginQuery(gpBeginQuery, (C.GLenum)(target), (C.GLuint)(id))
 }
-func BeginQueryIndexed(target uint32, index uint32, id uint32) {
+func BeginQueryIndexed(target uint32, index uint32, id Query) {
 	C.glowBeginQueryIndexed(gpBeginQueryIndexed, (C.GLenum)(target), (C.GLuint)(index), (C.GLuint)(id))
 }
 
@@ -8760,121 +8774,121 @@ func BeginTransformFeedback(primitiveMode uint32) {
 }
 
 // Associates a generic vertex attribute index with a named attribute variable
-func BindAttribLocation(program uint32, index uint32, name *uint8) {
+func BindAttribLocation(program Program, index uint32, name *uint8) {
 	C.glowBindAttribLocation(gpBindAttribLocation, (C.GLuint)(program), (C.GLuint)(index), (*C.GLchar)(unsafe.Pointer(name)))
 }
 
 // bind a named buffer object
-func BindBuffer(target uint32, buffer uint32) {
+func BindBuffer(target uint32, buffer Buffer) {
 	C.glowBindBuffer(gpBindBuffer, (C.GLenum)(target), (C.GLuint)(buffer))
 }
 
 // bind a buffer object to an indexed buffer target
-func BindBufferBase(target uint32, index uint32, buffer uint32) {
+func BindBufferBase(target uint32, index uint32, buffer Buffer) {
 	C.glowBindBufferBase(gpBindBufferBase, (C.GLenum)(target), (C.GLuint)(index), (C.GLuint)(buffer))
 }

Since this is a breaking change, feedback would be appreciated.

[dmitshur]

I suspect it's going to be quite difficult to get the confidence that making a breaking change as significant as this is going to be worth it at this point. Furthermore, such a decision invites further questions: "if we're willing to break the API for this, what other breaking API changes should we consider?" Given how long go-gl/gl has been around in its current form, I suspect it might be better to consider its API v1-like and stable, and leave significant breaking API changes to be done at another import path.

There already exist some alternative OpenGL APIs that try to be higher level and do use named types for OpenGL types like Buffer and Texture. For example, see golang.org/x/mobile/gl and github.com/goxjs/gl. Maybe it's better to use something like that instead if that is the API you want yourself? go-gl/gl itself tries to be as simple and low-overhead as possible.

Thanks. Also CC @hajimehoshi.

[myaaaaaaaaa]

Regarding backwards compatibility, I would argue that since go-gl/gl consists of automatically generated bindings of an effectively frozen spec, nearly all updates to go-gl/glow would involve a change in how the API is generated.

This is in contrast to more normal libraries that typically change the underlying implementation of a much smaller API, which lends itself more to the standard Go practice of backwards compatibility.

With that in mind, I think a perpetually unstable v0 may actually be more appropriate for go-gl/gl due to its unique circumstances. Due to how Go modules work, users can always avoid updating if they would like to guarantee non-breaking changes.

That said, after thinking about this some more, it is possible to ease the transition- how about just providing type aliases for now?

 type (
-	Buffer            uint32
+	Buffer          = uint32
 	...
 )

This would give people time to update their code before changing it to a full type declaration (which would be a breaking change)

Furthermore, such a decision invites further questions: "if we're willing to break the API for this, what other breaking API changes should we consider?"

My own position is that go-gl/gl's API should mirror the C API as closely as possible, while adapting the constructs that are in the spec but not in C (such as the typed objects in this PR, the typed enums mentioned in #86 , or changing the const char* -> uint8* rules to const char* -> string).

(All this is to say that I personally would have preferred a breaking API change over the gl.*WithOffset overloads, since the overloads cause go-gl/gl to diverge from the OpenGL spec)

There already exist some alternative OpenGL APIs that try to be higher level and do use named types for OpenGL types like Buffer and Texture. For example, see golang.org/x/mobile/gl and github.com/goxjs/gl. Maybe it's better to use something like that instead if that is the API you want yourself? go-gl/gl itself tries to be as simple and low-overhead as possible.

Thanks for the pointers. From a cursory glance, they don't seem to involve code generation?

I think the biggest appeal of go-gl/gl is that it's automatically generated from the official OpenGL specifications, which means that development efforts can focus on bug fixes and API translation rules rather than reimplementing API wrappers.

[hajimehoshi]

I agree that we should not introduce breaking changes in go-gl/gl even though this is still in v0, as go-gl/gl is already widely used. I don't know how this organization manages stable branches and development branches, so I think we need more discussion.

[pwaller]

It looks to me that we never got around to using tags, most of the code was written before Go had integrated version management. Given that the code has been around for a long time without major changes, I'd call what we have currently v1, and if there is a compelling need to make breaking changes, call that v2.

[myaaaaaaaaa]

It looks to me that we never got around to using tags, most of the code was written before Go had integrated version management. Given that the code has been around for a long time without major changes,

...

That sounds convincing enough.

In that case, how about adding (unstable) glx/ paths that live side-by-side with the (stable) gl/ paths? glx/ would have the freedom to make breaking API changes, while gl/ would be fully backwards-compatible.

Portability between the two would be provided on a best-effort basis (for example, type declarations in glx/ would just be aliases in gl/, allowing for easier migration)