threeify icon indicating copy to clipboard operation
threeify copied to clipboard

Clean code

Open Astrak opened this issue 5 years ago • 6 comments

Next steps for a beautiful code base:

  • [x] implement commitizen
  • [x] implement semantic-release
  • [x] finish the eslint/prettier setup (there are still a few snags but it the brunt of it is done in #18
  • [ ] setup linting for glsl files
  • [x] inspect the linting and "modernise" the code, by using the # for private members, avoiding null types (https://github.com/threeify/threeify/issues/39), using single object parameters (https://github.com/threeify/threeify/issues/40),
  • [x] setup a github action that verifies the linting for new commits
  • [x] remove all the any types necessary right now. A big one coming from threejs right now is the uniforms, that are all any.
  • [ ] remove additional warnings from the linter
  • [ ] can eslint understand vscode import sorting? Or does the Code extension for import sorting have a script to be run on CI? Currently eslint is silenced for imports but it still implements it and the packages for it are still there. If it's impossible, remove them.
  • [ ] tackle the typedoc part

Astrak avatar Jun 06 '20 16:06 Astrak

The problem with the glsl files is that our builder, tsconfig, doesn't package up glsl files. Thus I had to format them similarly to how Three.js currently does it. As a big string in a *.js file. It sucks because they are just a big string, thus no syntax highlighting, nor linting, nor auto-formatting (because it is not legal to reformat a JS string.)

I am not an expert on tsconfig-based building, but I know that webpack supports bundling of glsl files correctly. Thus it is likely best to switch back to webpack? I had initially gone with webpack, but @justinfagnani removed it: https://github.com/bhouston/threeify/pull/1

bhouston avatar Jun 06 '20 16:06 bhouston

GLSL is not an importable module type (yet, I've been working with standards to add CSS and HTML modules, GLSL might be another good target). I would probably recommend wrapping it into .js files with a build step - which doesn't have to be a bundler, it can be a per-file wrapping operation - or using a tagged template literal to hint to tools that they can highlight and format the glsl.

See this VS Code extension that highlights code in glsl`` tags: https://marketplace.visualstudio.com/items?itemName=boyswan.glsl-literal

justinfagnani avatar Jun 06 '20 19:06 justinfagnani

I just had a look at BabylonJS, where the glsl files are on their own and imported as is. But their setup looks a bit more tricky and I don't understand it all. They are on webpack + file-loader. https://github.com/BabylonJS/Babylon.js/blob/master/materialsLibrary/src/fire/fireMaterial.ts#L20

Astrak avatar Jun 06 '20 19:06 Astrak

Sketchfab's OSGjs does it this way https://github.com/cedricpinson/osgjs/blob/master/sources/osgShader/shaderLib.js

code:

import shader from 'osgShader/node/shader.glsl';

...

shaderSource: shader

webpack.config.js:

module: {
  loaders: [{
    test: /\.(frag|vert|glsl)$/,
    loader: 'raw-loader'
  }]
}

Astrak avatar Jun 06 '20 20:06 Astrak

I posted on twitter to get more input.... https://twitter.com/BenHouston3D/status/1269386322323607558

bhouston avatar Jun 06 '20 21:06 bhouston

I think with the raw *.glsl files as they are right now, we could add a GLSL linter on them. It will likely not be able to follow the include structure, but at least it will catch the obvious errors.

bhouston avatar Jun 15 '20 21:06 bhouston

I think most of this is now done.

bhouston avatar Feb 03 '23 13:02 bhouston