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

Wasm support and some cleanup. #18

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@dbriemann
Copy link

dbriemann commented Oct 28, 2018

Hi,

first of all I don't expect this to get merged already!

This PR adds wasm support via github.com/gopherjs/gopherwasm/js to this package but js via gopherjs should also still work. I also removed a few gl constants that are undefined in webgl. And last I changed TexImage2D to accept a []byte slice as wished in #11 .

Note that the new function (c *Context) initGlConstants() is needed because wasm does not support struct tags. It uses reflection because else we would need to list all gl constants again. If you think this would be better that's no problem to change it. I already have the equivalent function generated and ready.
The struct tags are removed because Gopherjs makes trouble if you try to set a value for a field with struct tag (probably a bug). But we don't need them anyways.

Let me know what you think!

@hajimehoshi
Copy link
Member

hajimehoshi left a comment

sgtm

canvas := document.Call("createElement", "canvas")
if canvas == js.Null() {
fmt.Println("CANVAS IS NULL")

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

Early return would be nice here.

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

OK. I was just using the existing example but yea sounds like an improvement. Will add that.

webgl.go Outdated
gl := canvas.Call("getContext", "webgl", attrs)
if gl == nil {
if gl == js.Null() {

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

Can this be js.Undefined()? I'm not confident on this.

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

Probably should be!

webgl.go Outdated
gl = canvas.Call("getContext", "experimental-webgl", attrs)
if gl == nil {
if gl == js.Null() {

This comment has been minimized.

Copy link
@hajimehoshi

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

See above.

}
```

webgl_example.html:
js_example.html:

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

Why was it renamed?

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

I renamed it to make a clearer distinction between the JS and the WASM example. Webgl seemed ambiguous.

webgl.go Outdated
for i := 0; i < num; i++ {
field := fields.Field(i)
// Ignore the embedded js.Value. All other fields are gl constants.
if field.Name != "Value" {

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

Early return would make this more readable:

if field.Name == "Value" {
  continue
}
// ...

Wdyt?

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

Sure for me both are fine. I can change it.

webgl.go Outdated
if field.Name != "Value" {
// Retrieve value from gl context.
jsval := ctx.Get(field.Name)
if jsval.Type() == js.TypeNumber {

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

When is the type not number? If we cannot expect, should it return error or just panic?

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

E.g. the type is not a number if the GL constant does not exist in webgl. I discovered a few of them through this and removed them from the constants above.
I checked for "not number" so we can just ignore all missing values. In fact there should be no missing values now. Maybe we should just panic here if something is missing?

This comment has been minimized.

Copy link
@hajimehoshi

hajimehoshi Nov 5, 2018

Member

Sure, I prefer panic since the constants in the spec must be obtained.

It looks like the removed constants might be defined in extensions?

This comment has been minimized.

Copy link
@dbriemann

dbriemann Nov 5, 2018

Author

The constants I removed are the following:

INFO_LOG_LENGTH
not needed in webgl.

NUM_COMPRESSED_TEXTURE_FORMATS
maybe related to https://developer.mozilla.org/en-US/docs/Web/API/WebGL_API/Compressed_texture_formats

SHADER_COMPILER
no idea what this is

SHADER_SOURCE_LENGTH
not needed in webgl

STENCIL_INDEX
not in spec. see KhronosGroup/WebGL#2370 (comment)

@dbriemann

This comment has been minimized.

Copy link
Author

dbriemann commented Nov 8, 2018

@hajimehoshi what do you think about the current state?

@hajimehoshi
Copy link
Member

hajimehoshi left a comment

lgtm, I'd like to know @dmitshur's opinion

@dmitshur

This comment has been minimized.

Copy link
Member

dmitshur commented Apr 11, 2019

Sorry about the slow response.

My concern here is that this project doesn't have an active owner/maintainer. Adding more functionality here makes it even harder to maintain.

This was the very first webgl binding for GopherJS and it has served well, but by now, there are other more active packages that provide webgl access, so I think a good direction could be to freeze and archive this package instead. Per gopherjs/gopherjs#894, we have limited resources.

If the Wasm port lived in another repository where it's more clear who develops/maintains it, that might be a more manageable solution. It would be less constrained by unnecessary backwards compatibility.

Alternatively, if we can add a maintainer to this repo. But I don't know what it means to be breaking API compatibility here, especially after this package has inactive for so long.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.