xinaesthete
commented
2 years ago I think it would be easier to review this if there was a single clean commit with the salient changes. Even comparing the state of your fork after rolling back the refactor, there are a lot of style changes (different indentation etc). Thanks for your efforts, I'm just trying to track down a down-stream bug and am finding it a little bit difficult to assess whether this might be relevant.
Updated all shaders to check for WebGL 2 context on the canvas element before compiling. If a WebGL 2 context is found, Open GL 3.0 syntax is inserted at the beginning of each shader. Code is also mildly style updated to utilize template literals.
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
Did you test your solution?
Problem Description
As noted in #38, shaders have been broken since THREE.js r118. I believe it is because these newer THREE.js versions are expecting OpenGL ES 3.0 syntax in their shaders.
Solution Description
I added a check in each shader to look for WebGL 2 rendering context on the canvas. If no WebGL rendering context is found, the shaders remain more or less untouched (see below). Otherwise, if WebGL 2 context is found, the shaders use template literals to insert OpenGL ES 3.0 syntax at the head of each shader.
Template literals are also now used throughout the shaders for ease of use and readability.
The only other slight change is found in the vertex shaders. They now import position as a vec3 attribute, inserting the attribute into a vec4 inside the main function. I believe this change is inconsequential, merely adopting the format of the standard THREE.js shaders.
Side Effects, Risks, Impact
I found no side effects when testing this in my own projects as well as internally testing the npm scrips in the package.json.
Aditional comments:
I believe that the tests in this repo are actually using a much later version of THREE.js than the dependency found in the package.json. I was playing around with updating a lot of these dependencies before committing to these specific shader changes, and I couldn't figure out why updating THREE.js to the latest version wasn't demanding OpenGL ES 3.0 on the shaders (which is actually what led me to think of adding a WebGL 2 check on the canvas before compiling each shader). I traced it to the three-orbit-viewer, and from there, three-orbit-controls. Updating the dependencies on those didn't seem to break anything, but it also didn't generate a WebGL 2 canvas either, so I ended up leaving that dependency rabbit hole for the time being in the interest of doing something more productive.
Hopefully this update will allow people using later versions of THREE.js to use the repo out of the box, without needing to dig into the shaders unless they want to, while also allowing people using currently using this repo with older THREE.js versions to continue updating it without breaking any functionality.
PS the first commit also included changes to index.js and load.js, which I rolled back and am including in a separate PR, as they aren't related to the shader changes at all.