Moving matrix multiplication to the CPU. Adding glMatrix dependency. #315
Conversation
|
@redwoodfavorite Can you add containers for this so that I can see the benchmarks? Also, I doubt that people are actually using our shader for anything else but can you rewrite some of your commits that remove functions to have the "breaks:" tag? Since we will be auto generating the changelog this is important. |
|
Should the benchmarks prove a performance improvment, these look good. |
|
@redwoodfavorite can you post benchmarks for these changes? |
|
Addresses this issue: #250 (comment) |
|
I made a demo with some high vertex spheres on a grid. Here's the before: Here's the after: Honestly, they are nearly identical in performance which somewhat suprises me. This change's benefits though are that it enables more accurate normal maps #250 (comment) and great simplifies debugging as well as code complexity of the vertex shader. |
|
Hold on, I've got some changes to the fragment shader normal mapping I'd like to make before we merge this. |
|
even if the performance difference is minimal, +1 for moving logic to js for ease of understanding / debugging. |
|
Please forgive my ignorance here, but... |
|
@jd-carroll Good question. This is change is only moving the multiplication of the model matrix and the view matrix, and the generation of the normal matrix to the CPU. These are calculated in Javascript and passed to the GPU as uniforms. With this method you have to calculate these matrices once per draw call, instead of per vertex, though the containers indicate the performance is about the same. The real advantage of this method however is that it lets you debug these matrices on the CPU, which is 100x easier. Also, this is a much more standard way of doing things. @FarhadG will probably tell you that it's difficult to find examples of normal matrix assembly in the vertex shader. |
|
Should be ready to go now. @FarhadG @adnan-wahab would you guys mind looking at this and running any test demos you guys have? |
| buffers.push( | ||
| { name: 'a_pos', data: vertices }, | ||
| { name: 'a_texCoord', data: textureCoords, size: 2 }, | ||
| { name: 'a_normals', data: normals }, |
There was a problem hiding this comment.
gonna have to be a_normal after @FarhadG's big ol change
|
@redwoodfavorite Can you look at the build breaking and rebase is need be? |
|
I like this (not sure about the glMatrix dependency though). @michaelobriena What's the priority of this? Do you want me to work on bringing this in? |
Enables:
Accurate normal mapping
Better performance for high vertex geometries