Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problematic to bind NULL values in webgl #20

Open
ptxmac opened this issue Nov 27, 2022 · 8 comments
Open

Problematic to bind NULL values in webgl #20

ptxmac opened this issue Nov 27, 2022 · 8 comments

Comments

@ptxmac
Copy link
Contributor

ptxmac commented Nov 27, 2022

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

@janpfeifer
Copy link
Collaborator

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 ?

@ptxmac
Copy link
Contributor Author

ptxmac commented Nov 28, 2022

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

@janpfeifer
Copy link
Collaborator

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.

@ptxmac
Copy link
Contributor Author

ptxmac commented Nov 28, 2022

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
Copy link
Contributor Author

ptxmac commented Nov 28, 2022

Created the first attempt in gowebapi/webidl-bind#11

@ptxmac
Copy link
Contributor Author

ptxmac commented Dec 5, 2022

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

@ptxmac
Copy link
Contributor Author

ptxmac commented Dec 12, 2022

@janpfeifer could to re-generate the files?

(Or should I create a PR with it?)

@janpfeifer
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants