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

Clean code #19

Closed
6 of 10 tasks
Astrak opened this issue Jun 6, 2020 · 7 comments
Closed
6 of 10 tasks

Clean code #19

Astrak opened this issue Jun 6, 2020 · 7 comments

Comments

@Astrak
Copy link
Collaborator

Astrak commented Jun 6, 2020

Next steps for a beautiful code base:

  • implement commitizen
  • implement semantic-release
  • finish the eslint/prettier setup (there are still a few snags but it the brunt of it is done in Apply linting #18
  • setup linting for glsl files
  • inspect the linting and "modernise" the code, by using the # for private members, avoiding null types (null? #39), using single object parameters (Objects as argument? #40),
  • setup a github action that verifies the linting for new commits
  • 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
@bhouston
Copy link
Owner

bhouston commented Jun 6, 2020

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: #1

@justinfagnani
Copy link
Contributor

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

@Astrak
Copy link
Collaborator Author

Astrak commented Jun 6, 2020

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
Copy link
Collaborator Author

Astrak commented Jun 6, 2020

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'
  }]
}

@bhouston
Copy link
Owner

bhouston commented Jun 6, 2020

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

@bhouston
Copy link
Owner

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
Copy link
Owner

bhouston commented Feb 3, 2023

I think most of this is now done.

@bhouston bhouston closed this as completed Feb 3, 2023
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

No branches or pull requests

3 participants