Shader parse error on working shaders

[CallumBugajski]

Distribution

Linux x64 APT package

Bug description

When compiling a pack that contains Shaders from objmc (https://github.com/Godlander/objmc), I've been getting errors from Shaders that work correctly within Minecraft. Seems like they cannot be parsed but are correctly formatted.

Output:

PackSquash v0.3.1-8-g349eaf0 (release, 2022-04-14) for x86_64-unknown-linux-gnu
Minecraft resource and data pack optimizer (CLI)

This program comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions. Use the -v command line switch for
more details about these conditions.

- Reading options from file.toml...
- Options read. Processing pack...
- Working around automatically detected Minecraft quirks: bad_entity_eye_layer_texture_transparency_blending
! assets/minecraft/shaders/include/objmc_light.glsl: Shader parse error: error: 0: at line 5:
  if (isCustom == 0) {color *= vertexColor;}
  ^
  expected ';', found i
  
  1: at line 5, in Alt:
  if (isCustom == 0) {color *= vertexColor;}
  ^
  
  2: at line 1, in Many1:
  //objmc
  ^
  
  
! assets/minecraft/shaders/include/objmc_main.glsl: Shader parse error: error: 0: at line 4:
  isCustom = 0;
  ^
  expected ';', found i
  
  1: at line 4, in Alt:
  isCustom = 0;
  ^
  
  2: at line 1, in Many1:
  //objmc
  ^
  
  
! assets/minecraft/shaders/core/render/block.vsh: Shader parse error: error: 0: at line 43:
      #define BLOCK
      ^
  expected '}', found #
  
  
! assets/minecraft/shaders/core/render/entity_overlay.vsh: Shader parse error: error: 0: at line 52:
      #define ENTITY
      ^
  expected '}', found #
  
  
! assets/minecraft/shaders/core/render/entity.fsh: Shader parse error: error: 0: at line 36:
      #define ENTITY
      ^
  expected '}', found #
  
  
> assets/minecraft/shaders/include/fog.glsl: Minified
> assets/minecraft/shaders/core/rendertype_cutout.json: Minified
> assets/minecraft/textures/block/outworld.png: Optimized with 100% quality color quantization
> assets/minecraft/textures/block/outgui.png: Barely optimized. If not optimized externally, try tweaking options for extra savings
! assets/minecraft/shaders/core/render/entity.vsh: Shader parse error: error: 0: at line 50:
      #define ENTITY
      ^
  expected '}', found #
  
  
> assets/minecraft/shaders/core/rendertype_translucent.json: Minified
> assets/minecraft/textures/block/outhand.png: Optimized with 100% quality color quantization
> assets/minecraft/shaders/core/rendertype_item_entity_translucent_cull.json: Minified
> assets/minecraft/shaders/include/objmc_tools.glsl: Minified
> assets/minecraft/shaders/core/rendertype_solid.json: Minified
> assets/minecraft/textures/block/shuba.png: Optimized with 93% quality color quantization
> assets/minecraft/textures/block/cube.png: Optimized with 97% quality color quantization
! Pack processing error: An error occurred while processing a pack file
  Another error message with more information was emitted before. You might need to scroll up to see it.
  These troubleshooting instructions might be useful: <https://packsquash.page.link/Troubleshooting-pack-processing-errors>

Reproduction steps

1. Download the following folder from objmc (the resource pack example): https://github.com/Godlander/objmc/tree/main/objmc
2. Use the pack squash command on the downloaded folder with the basic config file:

pack_directory = 'PATH_TO_PACK'
output_file_path = 'PATH_OUTPUT'
zip_spec_conformance_level = 'disregard'

Expected behavior

The pack should compile normally

Attached files

No response

Additional context

No response

[AlexTMjugador]

Hi! Thanks for reaching out to report your issue 😄

As it happens with JSON files, the GLSL parser used by PackSquash is more strict than Minecraft. However, unlike with JSON files, I did not intend the GLSL parser to be that way, so I agree that PackSquash should improve in this regard, and understand why it may be seen as a "bug". (There is a point in enforcing strict JSON syntax, because JSON is meant to be a data exchange format, and Minecraft accepts deviations that other commonly used programs do not. However, GLSL is rather application-specific, especially when considering the fact that Mojang extended it with preprocessor directives such as #moj_import.)

The errors you experience are caused by two known limitations, described in this issue comment:

• Preprocessor directives are only supported at the external declaration position (i.e. top level of a shader file). This means that PackSquash will spew errors if you use #moj_import inside a function, for example.
• Included shaders (.glsl files) are parsed as translation units (i.e. standalone, normal vertex or fragment shaders). Therefore, included shaders must only contain declarations. They can't contain expressions that would only be valid inside declarations, such as var = 3. Such included shaders are only valid if they are always #moj_import'ed within a declaration (function, data type definition, etc.), which is currently not possible due to the previous point, but it's still worth keeping in mind.

Luckily, these limitations can be worked around with no functional differences (i.e., without affecting how the shaders work):

• #moj_import directives can be replaced with the contents of the include shaders they import.
• Other preprocessor directives (i.e., lines that start with #) can be moved to the top level of the shader file.

I reckon that applying these workarounds at scale can be tedious, but they should be enough to get your pack to compile for now. Sadly, I don't expect them to be fixed soon, due to current technical constraints caused by how PackSquash is designed, which will be addressed for v0.4.0.

Please let me know if this comment was helpful to you.

[CallumBugajski]

Hi Alex,

Thank you for your quick reply, we'll give the workarounds a try and I'll let you know if we need anything else.

Thank you, Callum

[Godlander]

Some preprocessor directives cannot simply be moved to the top of files, such as #ifdef, #else and #endif. These are not Mojang extensions and are part of OpenGL Specifications.

Although without the use of #moj_import these can be worked around by modifying the code in each individual shader to only use the parts that would be included after the preprocessor.

[AlexTMjugador]

Some preprocessor directives cannot simply be moved to the top of files, such as #ifdef, #else and #endif. These are not Mojang extensions and are part of OpenGL Specifications.

Right, I wasn't thinking on those! Indeed, they are part of the specification. And, while the specification is not crystal-clear about it, I think that PackSquash is wrong here, because the specification makes several statements that equate the GLSL preprocessor with the one you'd find on C.

The only reason for this particular limitation to exist is that the fork of the third-party GLSL parser PackSquash uses has it. Upstream has been unresponsive for quite some time now and AFAIK there are no better GLSL parsers PackSquash could use, so any fix will likely be made by me or other PackSquash contributor on that fork. Sadly, time is finite for me now, and I'm prioritizing getting v0.4.0 released, but PRs are welcome 😄

[Godlander]

That's understandable, and it is possible to work around the limitation so it is not a big deal.

I believe it can be resolved by keeping the newlines around any line starting with #, but that might be a suboptimal solution.

[gp-Airee]

Is there a way to turn off the parsing of .glsl files or exempt specific files from processing?

[AlexTMjugador]

Is there a way to turn off the parsing of .glsl files or exempt specific files from processing?

I'm afraid that PackSquash does not provide any options to do that, sorry. Would you mind elaborating on why are you interested in disabling such parsing? Are the discussed workarounds impractical in your scenario?

Ideally, I would like to address this issue by fixing the PackSquash GLSL parser rather than letting people sweep its problems under the rug.

[Joshinn-io]

I am currently in your # | packsquash support channel in discord, but we're hoping to get the minify-shaders option to work to skip trying to parse glsl files. I sent the example of how we tried to use it, but it didn't seem to work.

[AlexTMjugador]

Note to self (and potentially other interested parties): the naga crate offers the GLSL validation and transpilation features required by PackSquash and seems much better maintained and widely used. It might be worth switching to it to get rid of the issue of restrictive preprocessor directive positions. glsl-lang looks like another interesting alternative. glslopt may be an interesting addition, but it is abandoned and may not work fine with Minecraft or newer GLSL constructs. glsl-include may be useful as a base to implement #moj_import resolution and proper validation in this case, although it's likely it can't be used as is due to PackSquash design constraints.

[UltraFaceguy]

I recently wrote some shaders and was very disappointed to realize I can't release an update because of this. Definitely is "at scale" enough for the level of frustration to workaround is too high. I support an option to skip shader validation. No reason not to throw in a skip if the validator currently has known issues validating the base OpenGL Specifications, at least until it's actually accurate.

[AlexTMjugador]

I am aware of the disappointing reality that some shaders can't be used as-is with the current PackSquash release. I am also aware that it a fix for the situation is warranted and necessary, especially in situations where refactoring shaders is not a viable option. I'm sorry for the inconvenience it causes to some users.

However, I won't add an option to skip parsing GLSL files. I don't believe that is the proper way to approach this issue for several reasons:

• The GLSL parser used by PackSquash is not black magic that no one can understand: as mentioned above, it could be changed to a better one without much effort to fix the most egregious issue of rejecting preprocessor declarations in most positions. It just has not been done yet, and at the time this feature was implemented there were no alternative parsers. My time is limited and, while shaders are important for some people, others value improvements in other regards more, such as performance, protection, ease of use and compression in other asset types. I want to make all of them happy, but that is not a trivial task.
• An additional option to skip parsing GLSL would default to some value. Or should it? If I make it mandatory, it'd be cumbersome for those who don't use shaders in their packs. Defaulting to not skip parsing would mean that people would still face this issue, and maybe even ask for support, before realizing that such an option exists. On the other hand, defaulting to skip parsing would render the minification feature useless by default, even though only a subset of shaders are affected by this issue (I know that some popular armor shader packs are affected by this, but they might not be representative of the whole population of shaders in the wild).
• Validating shaders aligns with my vision of what I want PackSquash to do. It'd be next to impossible to improve upon its compression capabilities without having more knowledge about specific data formats and their validity. In the long term, offering an escape hatch for validation is likely to be detrimental towards gaining such knowledge, as it renders it easier to form a "it just works this way" attitude towards things that is not technically constructive.

That being said, while I appreciate feedback and understand that the problem can be frustrating, I'd like to kindly ask further commentors to avoid further "I'm affected by this too" comments, unless they add something new and constructive to the topic. I want to make PackSquash as good as it can be 😄

[UltraFaceguy]

Well I can respect having a vison and wanting things to align to it, my vision as an end-user is to compress a valid resource pack.

All the reasoning in the world can't work around that fact that I have a valid formatted resource pack with GLSL spec compliant shaders and can no longer use this project to compress it. Even if only a "subset of shaders" are affected at the end of the day preventing a project from failing to perform it's core purpose over the prospect of a temporary config boolean seems a bit silly.

[AlexTMjugador]

I'm pleased to announce that I've just pushed a commit that allows PackSquash to support a wider variety of shaders, without resorting to changing options or throwing minification out of the window in the supported cases! 🎉

The technical details of how this was accomplished can be read at the commit description. In summary:

• PackSquash now supports prettifying GLSL files, in addition to minifying them or keeping their contents intact.
  • The source transformation behavior is controlled by the new shader_source_transformation_strategy option, which replaces minify_shader and can be set to minify, prettify or keep_as_is.
• PackSquash now has a GLSL preprocessor that expands and removes most preprocessor directives as a first step.
  • Expanding preprocessor directives may reduce file sizes, improve obfuscation, and catch #error directives being emitted during the optimization process.
  • However, it may also increase file sizes if e.g. preprocessor variables were used to alias repetitive code.
  • Only the #pragma, #extension and #moj_import directives are retained in minified or prettified GLSL files.
• The preprocessor allows PackSquash to accept preprocessor directives anywhere, not just in external declaration position.
  • The abstract syntax tree returned by the GLSL parser still only supports preprocessor directives in external declaration position, so PackSquash will not minify or prettify files that contain any of the #pragma, #extension or #moj_import preprocessor directives outside of such positions.
• Include shaders can now be standalone expressions and statements. Previously, PackSquash required them to be translation units (i.e., one or more external declarations).
  • Due to its new preprocessor directive expansion and removal step, PackSquash will avoid minifying or prettifying include shaders that contain preprocessor directives, as they may be relied upon by the shader that includes them.
• At the moment, five classes of shaders remain unable to be parsed:
  • Shaders which depend on #moj_imported preprocessor variables to be syntactically correct.
  • Include shaders which depend on preprocessor variables defined in the parent shader to be syntactically correct.
  • Shaders which depend on #moj_imported preprocessor variables to expand to the expected source code, but are syntactically correct otherwise. (This can be worked around by setting the new shader_source_transformation_strategy = 'keep_as_is' option for the affected shaders.)
  • Include shaders which depend on preprocessor variables defined in the parent shader to expand to the expected source code, but are syntactically correct otherwise. (This can be worked around like in the previous point.)
  • Shaders that include vertex or fragment shaders, relying on their preprocessor variables or syntax to be valid. (This can be worked around in some cases by setting is_top_level_shader = false for the included shaders, although including vertex or fragment shaders is an extreme edge case that I never saw before, and arguably a bad idea.)
• On top of that, the new GLSL parser and preprocessor claims to be between 5 and 480 times faster than the one previously used by PackSquash! 🚀

The new design of v0.4.0 allows removing at least the first and third classes of troublesome shaders, which means that v0.4.0 will be able to process most kinds of practical shaders automagically. (Edit: this redesign was scheduled for a later release.)

For now, I hope that these changes help at least some of you. You can try them out on the latest unstable builds. If you are using the PackSquash GitHub Action, you'll have to bring your own options file.

For those that were eager for an improvement in the situation, this represents my first concrete step forward 😉

[AlexTMjugador]

As announced on Discord, the roadmap for v0.4.0 has changed, and it will be released soon with the partial fix for this issue described above.

This does not affect my plans to release a better fix in the future, once the necessary redesign originally planned for v0.4.0 lands in a future version, such as v0.5.0. With the more complete fix is in place, I'd feel comfortable adding a last-resort option to skip parsing to handle use cases that might not be feasible to support.

Thank you all for your patience ❤️

[AlexTMjugador]

Due to the fact that the redesign required for proper #moj_import expansion and source transformation mentioned above is taking a while, and this issue has been causing significant churn for some people, I have just reconsidered my stance on the situation.

Since the latest commits, PackSquash is now very conservative about which shaders it is willing to minify or prettify, in order to avoid unwanted parsing errors or surprises when loading the pack in-game as much as possible. This means that any shader that uses #moj_import will now not be minified or prettified, but PackSquash should work with all the problematic shader classes mentioned earlier in this issue. Parsing errors for shaders that use #moj_import are now considered tentative, meaning they show up in the output but only cause a fallback to no source transformation, to allow pack authors to decide for themselves if the error is a false positive or not.

I'll continue to work on better expansion and transformation capabilities, but I'd rather track them as separate enhancements and issues, since the shaders should at least "just work" now.

In any case, please feel free to give feedback on the latest changes! It's very welcome 😄

[UltraFaceguy]

Saw this was back on the next release backlog, just as a note, v0.4.0 seems to now get tripped up on my tooltip shader, as well as some other random one that worked with the old minify_shader = false setting. Both of these shaders work fine in vanilla, and with the previous solution.

Tooltip shader:

vec2 pad = vec2(3,3);
ivec3 sizes = ivec3(1,1,6);
uint base = 0x191614e2u;
uint[] tl = uint[](0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0xae7d36ffu,0xc28530ffu,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xae7d36ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xca8c32ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u);
uint[] tr = uint[](0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0xc28530ffu,0xae7d36ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xae7d36ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xcd9341ffu,0x1e0d00ffu);
uint[] bl = uint[](0x1e0d00ffu,0xc28530ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xae7d36ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x191614e2u,0x191614e2u,0x1e0d00ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0xae7d36ffu,0xc28530ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu);
uint[] br = uint[](0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xcd9341ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xae7d36ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0x191614e2u,0x191614e2u,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0xc28530ffu,0xae7d36ffu,0xf2b129ffu,0xf2b129ffu,0xf2b129ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu,0x1e0d00ffu);
uint[] t = uint[](0x1e0d00ffu,0xca8c32ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u);
uint[] l = uint[](0x1e0d00ffu,0xca8c32ffu,0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u);
uint[] r = uint[](0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xd59943ffu,0x1e0d00ffu);
uint[] b = uint[](0x191614e2u,0x191614e2u,0x191614e2u,0x191614e2u,0xca8c32ffu,0x1e0d00ffu);

Snippet from the text_effects.config.glsl shader:

TEXT_EFFECT(251, 255, 255) {
    //override_text_color(rgb(255, 255, 255));
    remove_text_shadow();
}
TEXT_EFFECT(251, 251, 255) {
    apply_skewing_movement();
    //override_text_color(rgb(255, 255, 255));
}

[AlexTMjugador]

Hi @UltraFaceguy, did you try the latest unstable builds? They should be more lenient when it comes to handling shader parsing errors. The issue was closed because the user-visible errors were fixed in such builds.