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

performance, use webgl2 as default context #878

Merged
merged 4 commits into from
Aug 12, 2021

Conversation

hrgdavor
Copy link
Contributor

@hrgdavor hrgdavor commented Jul 17, 2021

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Does your submission pass tests?

webgl2 is currently at 77.49% https://caniuse.com/webgl2

webgl2 is mostly backward compatible, and main issue is that some extensions are now included, so it might require some compat tweaks.

Looks like just using webgl2 instead of webgl brings perf improvements out of the box https://emscripten.org/docs/optimizing/Optimizing-WebGL.html#which-gl-mode-to-target

threejs impl for this started in 2016 mrdoob/three.js#8125 so if there will be issues, it should be easy to find solution

It seems to be working just fine after some initial testing and loading models.

browser tests

tested on Windos 10 : Chrome, Firefox, Edge

@z3dev
Copy link
Member

z3dev commented Jul 17, 2021

Looks like a nice performance boost.

However, iOS browsers are not supported yet. It would be kind of bad to disable that group of users.

Hopefully, this will be supported soon.

@hrgdavor
Copy link
Contributor Author

hrgdavor commented Jul 17, 2021

@z3dev It is not disabling anything 🦊 ... just prefers webgl2, and if unavailable uses webgl.

the snippet is taken from regl source. I just added webgl2 on top. regl normally accepts canvas or context, and I was already was using this fact for offscreenCanvas in web worker rendering

 return (
    get('webgl2') ||
    get('webgl') ||
    get('experimental-webgl') ||
    get('webgl-experimental')
  )

@z3dev
Copy link
Member

z3dev commented Jul 17, 2021

@hrgdavor these changes look fine.

however, REGL should also be doing this internally. If not already then let’s raise a new issue.

@hrgdavor
Copy link
Contributor Author

@z3dev sure. I will look into it, so we get it via regl instead of this PR.

@hrgdavor
Copy link
Contributor Author

@z3dev I have opened an issue regl-project/regl#623 , and let's see if this is something they are willing to do in regl.

sidenote: regl-renderer currently uses version 1.6.1 of regl, so I will be trying out latest: 2.1.0

@z3dev z3dev self-requested a review July 18, 2021 17:49
@hrgdavor
Copy link
Contributor Author

I must sadly report that it is unlikely there will ever be webgl2 in regl 😞 regl-project/regl#378

I was doing initial impl for #812 and wanted to use oes_element_index_uint (uint32 for indices > 65535) and hit a brick wall with regl.

  • I can for example use regl+webgl1 and allow the extension
  • If I use regl+webgl2 I cant then use oes_element_index_uint because regl prevents using uint32 as it wrongly thinks iti is needed in webgl2
  • so now that I will have code that works for both those that have oes_element_index_uint and those that dont, regl forces me to disable oes_element_index_uint when using webgl2

#812 will still be useful to any renderer, as any of them using webgl will be able to use the buffers directly without further conversion.

I have no patience to fight with regl folks. I will rather spend time making an alternative renderer. I am sure that there will be things that will benefit regl-renderer performance in the process.

Threejs based renderer is working for me and I use it it constantly, and hopefully I can make it good enough to include in jscad at some point as alternative option. People then can switch if they want to use threejs or regl-renderer without issues. Some may prefer threejs, some may hate threejs as bloat. ( I am actually extracting code for #812 from my threejs renderer prototype)

I am making nice progress on web worker and it will likely provide reusable code that could be then used to make versions based on both regl and threejs.

@hrgdavor
Copy link
Contributor Author

hrgdavor commented Jul 20, 2021

What I am learning while researching this topic is that it will be easier to standardize renderer worker, and I am close to doing that regl-renderer and my prototype threejs-renderer are easily interchangeable.

The renderer does not have to support too many things, and it is easier to use regl with ppl that are ok with webgl1, and then let them have option to use threejs if the want benefits of webgl2.

This way we do not need a magic library that is small and supports both webg1 and webgl2.

Alsno there is new kid on the block promising even more performace: WEBGPU ... and for that one, it makes even more sense to implement renderer, rather than looking for lib that works seamless with webgl1, webgl2, webgpu. Especially as it has rather different API.

maybe you would enjoy some reading till sept 1st :)
https://hacks.mozilla.org/2020/04/experimental-webgpu-in-firefox/
https://eytanmanor.medium.com/the-story-of-webgpu-the-successor-to-webgl-bf5f74bc036a
http://austin-eng.com/webgpu-samples

@hrgdavor
Copy link
Contributor Author

s lIke support for oes_element_index_uint is at 95% globally and more likely 100% if the browser supports webgl at all (since 2015)
https://caniuse.com/mdn-api_oes_element_index_uint

I have a snippet that I use in my renderer worker that works around regl limitation

  function createContext (canvas, contextAttributes) {
    function get (type) {
      try {
        return {gl:canvas.getContext(type, contextAttributes), type}
      } catch (e) {
        return null
      }
    }
    return (
      get('webgl2') ||
      get('webgl') ||
      get('experimental-webgl') ||
      get('webgl-experimental')
    )
  }

.......
.......

    const {gl, type} = createContext(canvas)
    // prepare the renderer
    const setupOptions = {
      glOptions: {gl}
    }
    if(type == 'webgl'){
        setupOptions.glOptions.optionalExtensions = ['oes_element_index_uint']      
    }
    renderer = prepareRender(setupOptions)

@z3dev I would argue that it is safe to remove the limit for length of vertices array that is 65536

@z3dev
Copy link
Member

z3dev commented Jul 20, 2021

@hrgdavor once again, thanks for the research and information.

it seems that the extension is supported across the world of browsers so let’s try using using it.

And there’s a test case for HUGE geometries as well, so the conversation should work fine.

I see two options

  • only support rendering using the extension, and throw an error if the extension is not available (and ask the user to upgrade)
  • support both cases, and implement old and new conversions

But, we already have a ‘move forward’ policy. This extension has been fully supported for years. So, I would favor the first option. Users should be upgrading and moving forward.

@hrgdavor
Copy link
Contributor Author

So, I would favor the first option. Users should be upgrading and moving forward.

Excellent. I will update the PR to do what you asked:

  • only support rendering using the extension,
  • throw an error if the extension is not available (and ask the user to upgrade in the err mesage)

@hrgdavor
Copy link
Contributor Author

@z3dev it is ready for review when u get time
image

to cause the error you can edit viewer.js on line 232

  if (type === 'webgl') {

and put webgl2 instead of weblg ... this will cause the check to fail, as webgl2 does not have the extension (it is builtin)

@z3dev
Copy link
Member

z3dev commented Aug 6, 2021

@hrgdavor can you add a list of OS, browsers, and versions tested to the initial comment.

If needed then we can enlist some testing across more browsers.

@hrgdavor
Copy link
Contributor Author

hrgdavor commented Aug 6, 2021

@hrgdavor can you add a list of OS, browsers, and versions tested to the initial comment.

tested on Windos 10 : Chrome, Firefox, Edge

@z3dev z3dev requested a review from SimonClark August 7, 2021 16:18
Copy link
Member

@z3dev z3dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hrgdavor thanks again. These changes look fine.

let’s see if @SimonClark would like to review.

@hrgdavor hrgdavor changed the title performance, use webgl as default context performance, use webgl2 as default context Aug 12, 2021
@hrgdavor
Copy link
Contributor Author

@z3dev this one is important for finishing #812 which should bring significant performance gains.

@z3dev z3dev merged commit a6c92f9 into jscad:master Aug 12, 2021
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.

None yet

2 participants