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

Switch to web-sys instead of stdweb. #241

Merged
merged 9 commits into from
Sep 22, 2020
Merged

Switch to web-sys instead of stdweb. #241

merged 9 commits into from
Sep 22, 2020

Conversation

sebcrozet
Copy link
Owner

It appears that stdweb is mostly dead so I would like to switch to web-sys/wasm-bindgen instead. This is a breaking change because:

  • This completely removes the stdweb dependency in favor of web-sys.
  • This uses glow instead of our generated webgl/opengl bindings.

@alvinhochun
Copy link
Collaborator

I've given it a few tries and had commented on it on discord, so I'm repeating them here for completeness.

On web it works mostly fine but I found some issues:

  1. Right-click context menu isn't prevented from showing.
  2. After Esc is pressed (thus ending the Kiss3d render loop), I continuously get uncaught error "closure invoked recursively or destroyed already" in the console.
  3. Maybe one more (but might not be a regression): if the window is small enough that the canvas has a size of 0, it panics with "The aspect ratio must not be zero".

Running on native Windows results in an assertion failure panic in Effect::drop when closing the Kiss3d window.

@alvinhochun
Copy link
Collaborator

@sebcrozet You mentioned on discord that "glow does not have functions like is_shader, is_program etc.", can you open an issue on the glow repo to ask for them to be added?

@sebcrozet
Copy link
Owner Author

Thank you for your feedbacks @alvinhochun.

You mentioned on discord that "glow does not have functions like is_shader, is_program etc.", can you open an issue on the glow repo to ask for them to be added?

I opened a glow issue about the missing is_ functions.

Running on native Windows results in an assertion failure panic in Effect::drop when closing the Kiss3d window.

If I remember correctly this was fixed in earlier versions of Kiss3d by validating that the shaders/programs being dropped are valid using the is_shader/is_program functions. So it is likely the problem appears again because of the lack of validation function in glow.

Right-click context menu isn't prevented from showing.

Looks like something that should be configured in the <canvas style.

After Esc is pressed (thus ending the Kiss3d render loop), I continuously get uncaught error "closure invoked recursively or destroyed already" in the console.

Weird, will have to investigate this.

Maybe one more (but might not be a regression): if the window is small enough that the canvas has a size of 0, it panics with "The aspect ratio must not be zero".

This may be an assertion that did not exist before.

@alvinhochun
Copy link
Collaborator

Right-click context menu isn't prevented from showing.

Looks like something that should be configured in the <canvas style.

Oh, for some reason I thought the past stdweb version called e.preventDefault() but looks like both versions actually behave the same way. Not a regression I suppose.

Maybe one more (but might not be a regression): if the window is small enough that the canvas has a size of 0, it panics with "The aspect ratio must not be zero".

This may be an assertion that did not exist before.

Actually not a regression. The stdweb version also panics. Looks like it is caused by the camera code calling Perspective3::set_aspect() for the FramebufferSize event even when w is 0.0, so not related to this change. Should be fixed separately though.

@alvinhochun
Copy link
Collaborator

You mentioned on discord that "glow does not have functions like is_shader, is_program etc.", can you open an issue on the glow repo to ask for them to be added?

I opened a glow issue about the missing is_ functions.

Running on native Windows results in an assertion failure panic in Effect::drop when closing the Kiss3d window.

If I remember correctly this was fixed in earlier versions of Kiss3d by validating that the shaders/programs being dropped are valid using the is_shader/is_program functions. So it is likely the problem appears again because of the lack of validation function in glow.

#242 should fix these issues.

alvinhochun and others added 2 commits September 10, 2020 17:30
- Fix glow API changes
- Added a few `verify!` calls
- Move `Window::canvas` field to the end so that the GlWindow will not
  be dropped before other structs that requires the OpenGL Context in
  their `Drop` impl.
(web-sys) Update to glow 0.6
@alvinhochun
Copy link
Collaborator

alvinhochun commented Sep 17, 2020

After Esc is pressed (thus ending the Kiss3d render loop), I continuously get uncaught error "closure invoked recursively or destroyed already" in the console.

@sebcrozet I suppose the only remaining blocker is this? I'm looking into it. PR #247 should fix this issue.

@sebcrozet
Copy link
Owner Author

sebcrozet commented Sep 22, 2020

Fixed the CI. Let's merge this! thanks @alvinhochun for the additional fixes 💯

@sebcrozet sebcrozet merged commit 68a62da into master Sep 22, 2020
@sebcrozet sebcrozet deleted the web-sys branch September 22, 2020 07:27
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

Successfully merging this pull request may close these issues.

2 participants