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

Changes and fixes to grammar: Number literals, preprocessor, string #57

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

akberg
Copy link

@akberg akberg commented Apr 29, 2024

I've been searching hopelessly for a remotely useful GLSL language support for VSCode for a while, and this one immediately seems to provide a mostly decent highlighting. I did however notice a few misses, that need fixing -- and added an extra which is open for discussion:

  • Fixed the regex for fixed point numeric literals (which was failing on hex numbers due to precedence issues), leaving it more comprehensive.
  • Fixed regex and naming of preprocessor directives to highlight only valid directives, to work with a larger set of themes, and to allow advanced themes to highlight preprocessor directives differently from other keywords.
  • For discussion: Some GL extensions (e.g. GL_GOOGLE_include_directive) provide the include directive for including files and reusing code. For this, I've added the include keyword to the preprocessor list, and a rule for double-quoted strings. I'm not using GLES though, just GL, so I don't know how available those extensions are for ES. In any case, supporting the highlighting would make the extension more accessible, given that this is pretty much the only useful syntax highlighter on the marketplace.

akberg added 5 commits April 29, 2024 14:14
The regex was not properly catching hex numbers.
Modified the regex to be more comprehensive and
fix the issue.

Signed-off-by: Andreas K. Berg <[email protected]>
There are extensions that add support for file includes
in GLSL. Not sure about the extent to which those are
used or available in ES, but including the rule makes
the extension more accessible for a larger audience.

Signed-off-by: Andreas K. Berg <[email protected]>
Added the defined preprocessor keywords (+include, see previous
commit for suggestion), instead of whatever kind of black magic
that was blocking theming of directive parameters. Now, only
a valid preprocessor keyword will be highlighted, and any literals
or symbols behind it will also be properly highlighted. Also
changed the scope name to hit better with common themes (Tested
with Microsoft themes, which colour preprocessor directives
differently, as well as with variants of Atom and Material and
others).

Signed-off-by: Andreas K. Berg <[email protected]>
Signed-off-by: Andreas K. Berg <[email protected]>
Fixed error introduced in previous commit.

Signed-off-by: Andreas K. Berg <[email protected]>
@racz16
Copy link
Owner

racz16 commented Apr 30, 2024

Hi, thanks for contributing.

Fixed point numbers

You're right, it was a mistake, thanks for fixing it. My only nitpick is that in all other places, I use \\d, instead of 0-9, so please, change that back.

Preprocessor directives

Preprocessor directives are a bit more complicated. While it's not a bad idea to color preprocessor directives basically as keywords and the content as regular GLSL code, your implementation has a couple of problems.

  • It's a bit strange and misleading that if I start to write a directive, at first, it'll be colored as a directive (because # alone is a valid directive), then the coloring breaks, because #def is not a valid directive, and when I finish #define, it colors the code correctly again. When I type, I feel like I made a mistake, because coloring changes.
    image

  • If a new language version or extension adds a new preprocessor directive to GLSL, the coloring won't be right.
    image

  • Your implementation accepts ## as a valid directive.
    image

  • It can't handle comments before directives. It can't handle comments between the # and the directive's name, but the current implementation can't handle it either.
    image

  • It can't handle the error directive's message.
    image

  • It colors defines in 3 different ways.
    image

  • Maybe it would be a good idea to color special items as constant.language.glsl, like es, on, off, all, required, warn, disable, all, STDGL, optimize, debug, maybe there are some others.
    image

Includes and string

My extension only supports WebGL-related GLSL versions: GLSL ES 1, and GLSL ES 3. Since these versions don't support the include directive neither as a core feature, nor as an extension, I don't really want to support it. First of all, in WebGL, you have to use the browser's built-in shader compiler, which means, includes won't work. You can't use some other compiler, or generate SPIR-V. So while this might work in VS Code, it wouldn't work in the browser. But even if the syntax highlight works with includes, all other features wouldn't. The compiler would mark it as an error, you wouldn't get any items in code completion from the included files, etc. Also, I don't support other, features that are not available in WebGL, like compute shaders, SSBOs, etc.

On the other hand, I started to work on a new extension, which uses the Language Server Protocol, and it'll support all 17 GLSL versions, it'll properly handle preprocessor directives for example the include, and it'll work in Visual Studio as well. Sadly, at the moment I don't really have the time to work on it, but eventually, it'll be the successor of this extension, and hopefully, it'll solve all problems.

@racz16
Copy link
Owner

racz16 commented May 27, 2024

@akberg, will you continue to work on this Pull request?

@akberg
Copy link
Author

akberg commented May 28, 2024

I'll push the additional edits I made that make me more happy to use the extension, but if, as you say, you intend to have this extension's support limited to ES, then I should take my contributions somewhere else - my interest is in having general GLSL support compiled in one place, and currently compute shaders in particular.

For processor directives, I just intended to do an initial fix, so just keeping the changed scope name would suffice to fix syntax highlighting.

  • I intended to only highlight valid keywords (## is valid with the GLSL spec), but if extensions may add directives I agree that just all words should be highlighted, and a semantic highlighter would be necessary to do any better.
  • Block comments in between directives should be easy enough to fix, I wasn't conscious about it (and even Microsoft's C/C++ extensions don't really have good support for it).

Thanks for taking the time to properly review, btw.

@akberg
Copy link
Author

akberg commented Jun 4, 2024

Thanks for taking the time to properly review. I did some more local changes for the highlighting to be good enough for my using, but if, as you say, you intend to have this extension's support limited to ES, then I should take my contributions somewhere else - my interest is in having general GLSL support compiled in one place, and currently compute shaders in particular.

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

Successfully merging this pull request may close these issues.

2 participants