webapi icon indicating copy to clipboard operation
webapi copied to clipboard

Problematic to bind NULL values in webgl

Open ptxmac opened this issue 3 years ago • 8 comments

A typical pattern in WebGL is to bind a "null" value.

E.g.

gl.bindFramebuffer(gl.FRAMEBUFFER, null);

According to the documentation, this is how to reset the current FBO back to the canvas:

framebuffer A WebGLFramebuffer object to bind, or null for binding the Canvas or OffscreenCanvas object associated with the rendering context.

Because of how the types are implemented in WebGL, this is not elegant to do

Preferred pattern:

gl.BindFramebuffer(webgl.FRAMEBUFFER, webgl.FramebufferFromJS(js.Null()))

Unfortunately this will panic

Problem

From webgl_js.go:

// FramebufferFromJS is casting a js.Value into Framebuffer.
func FramebufferFromJS(value js.Value) *Framebuffer {
	if typ := value.Type(); typ == js.TypeNull || typ == js.TypeUndefined {
		return nil
	}
	ret := &Framebuffer{}
	ret.Value_JS = value
	return ret
}

In the case of js.Null() this returns a nil pointer, which we try to pass to BindFramebuffer

func (_this *RenderingContext) BindFramebuffer(target uint, framebuffer *Framebuffer) {
	var (
		_args [2]interface{}
		_end  int
	)
	_p0 := target
	_args[0] = _p0
	_end++
	_p1 := framebuffer.JSValue()
	_args[1] = _p1
	_end++
	_this.Value_JS.Call("bindFramebuffer", _args[0:_end]...)
	return
}

Unfortunately, this code expected a non-nil Framebuffer, and will panic

Workaround

It's possible to work around it by manually creating the Framebuffer object, but it's not elegant:

nullFBO := &webgl.Framebuffer{Object: webgl.Object{
	Value_JS: js.Null(),
}}
gl.BindFramebuffer(webgl.FRAMEBUFFER, nullFBO)

Solutions

I can think of a couple of solutions:

  1. Simple remove the IsNull check from the *FromJS methods and allow the creation of null objects. gl.BindFramebuffer(webgl.FRAMEBUFFER, webgl.FramebufferFromJS(js.Null()))
  2. Create global Null versions of objects. gl.BindFramebuffer(webgl.FRAMEBUFFER, webgl.NullFramebuffer)
  3. Change the bind methods and convert nil to js.Null gl.BindFramebuffer(webgl.FRAMEBUFFER, nil)

Notes

From my OpenGL understanding this pattern is used for all objects. I.e. all Bind* and Use* methods (gl.bindTexture/gl.useProgram/gl.bindBuffer etc)

The mozdev webgl documentation is unfortunately a little lacking, and only framebuffers are explicitly mentioned to be null - but I assume it also holds true for any other objects

ptxmac avatar Nov 27 '22 14:11 ptxmac

Thanks for the detailed report Peter.

I haven't used it in a while (but I'm planning to next year), but another solution that comes to mind, to preserve the interchangeability between Go's nil and JS's Null, would be changing the .JSValue() method to convert nil to the a js.Null. So the line below would work:

func (_this *RenderingContext) BindFramebuffer(target uint, framebuffer *Framebuffer) {
    ...
    _p1 := framebuffer.JSValue()
    ...

What do you think ? Would that work ?

janpfeifer avatar Nov 27 '22 19:11 janpfeifer

I think that's more or less what I meant with my 3rd suggestion. I.e. from the users perspective, it would look like the following?

gl.BindFramebuffer(webgl.FRAMEBUFFER, nil)

(edited)

So you're suggesting making a change like this?

func (_this *Object) JSValue() js.Value {
+   if this == nil {
+      return js.Null()
+   }
    return _this.Value_JS
}

Looks good to me, I can't think of any time where a panic is preferred to js.Null 😄

The only thing to be mindful of is null vs undefined - I don't think there's anywhere in WebGL where you would use undefined, but that's a little outside my experience

ptxmac avatar Nov 28 '22 09:11 ptxmac

Yes, that's what I had in mind.

I'm also no JavaScript expert, so I don't have an opinion on the null vs undefined choice. But as you said, better than panic.

Do you want to write the PR ? I'll approve it. I would just ask to add a line of comment on the JSValue() method about nil being converted to null in JavaScript.

janpfeifer avatar Nov 28 '22 11:11 janpfeifer

Do you want to write the PR ? I'll approve it. I would just ask to add a line of comment on the JSValue() method about nil being converted to null in JavaScript.

Yup, I'll take a stab at it

ptxmac avatar Nov 28 '22 12:11 ptxmac

Created the first attempt in https://github.com/gowebapi/webidl-bind/pull/11

ptxmac avatar Nov 28 '22 14:11 ptxmac

This can now be resolved if the files are re-generated

ptxmac avatar Dec 05 '22 09:12 ptxmac

@janpfeifer could to re-generate the files?

(Or should I create a PR with it?)

ptxmac avatar Dec 12 '22 14:12 ptxmac

hi @ptxmac, sorry I missed it. I was hoping you would re-generate the files as well. I only did it once, > 1 year ago, I would have to investigate how to do it.

I'm currently finishing a different project that doesn't use gowebapi. I'm planning to get back to it next year, in a couple of months.

janpfeifer avatar Dec 12 '22 20:12 janpfeifer