Skip to content
This repository was archived by the owner on Aug 7, 2023. It is now read-only.

[WIP]glslify support - #68

Open
fand wants to merge 8 commits into
AtomLinter:masterfrom
fand:glslify
Open

fand wants to merge 8 commits into
AtomLinter:masterfrom
fand:glslify

Conversation

@fand

@fand fand commented Aug 16, 2017

Copy link
Copy Markdown

It's still experimental.
Please give me some advice if you are interested 😸

glslify/glslify#95

Changes

  • Added Use glslify to config
  • Uses forked version of glslify to generate sourcemaps

@fand

fand commented Aug 16, 2017

Copy link
Copy Markdown
Author

It parses sourcemaps and shows the error line (almost) correctly.

2017-08-16 13 31 55

Comment thread package.json
"atom-message-panel": "^1.2.4",
"atom-package-deps": "^4.3.1",
"convert-source-map": "^1.5.0",
"glslify": "https://github.com/fand/glslify#sourcemaps",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure I really want to use a branch on a fork here, that's just led to outdated and unmaintained code in every previous case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea, this is just an experimental fork to get sourcemaps and disable minification.
I'm gonna replace it with glslify if it supports sourcemaps.

Comment thread lib/linter-glsl.js
default: false,
order: 2,
},
useGlslify: {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This probably should have a description on here describing what this is actually doing.

Comment thread lib/linter-glsl.js Outdated
const colStart = parseInt(match[2], 10);
const lineEnd = lineStart;
const colEnd = colStart;
let l = parseInt(match[3], 10);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just use lineStart and colStart as the original code, there is no reason to switch to obscure names only to put them back to understandable ones right after.

Comment thread lib/linter-glsl.js Outdated
}
if (this.useGlslify) {
filesToValidate.forEach((f) => {
const baseDir = f.fullFilename.replace(/\/[^/]*$/, '');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this just the equivalent of path.dirname?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yep, replaced it with path.dirname 🙇

Comment thread yarn.lock Outdated
@@ -0,0 +1,1663 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm very much not a fan of yarn, having had it break too many projects. Please remove this file from the PR, thanks!

@Arcanemagus

Copy link
Copy Markdown
Member

This is a very interesting idea, I'm not too sure about moving forward with it here though until support is more baked into the tools behind this.

Thoughts @andystanton?

@andystanton

Copy link
Copy Markdown
Member

@fand @Arcanemagus I think this is a fantastic idea.

As you've already discussed, the sourcemap work needs to be merged in glslify first, but then we can definitely take a look at the best way to integrate it.

I would embrace making glslify linting the default as it will work out of the box, but still give the user the option to use glslangValidator if they prefer to have official Khronos validation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants