Broken shaders on Atlantica map

[dionrhys]

This is an issue with Atlantica's shader file because it uses '#' character comment blocks which are not valid in a shader file.

For example:

####################
////// System //////
####################

textures/atlantica/atlantica_softclip
{
    qer_editorimage textures/system/clip
    q3map_material LongGrass
    q3map_notjunc
    q3map_nolightmap
    qer_trans   0.5
    surfaceparm noimpact
    surfaceparm nomarks
    surfaceparm nodraw
    surfaceparm nonsolid
    surfaceparm nonopaque
    surfaceparm playerclip
    surfaceparm monsterclip
    surfaceparm trans
}

Here are some screenshots showing how this affects the map:

The retail version of JKA handles this invalid syntax fine somehow by ignoring them, but OpenJK's new shader code chokes on it and doesn't handle the first shader it finds after a #################### line properly. This means that invalid lines will break whatever shader is supposed to come afterwards, and no warnings are given for this happening.

I'd like a discussion on whether the JKA behaviour of ignoring (liberally-parsing) bad syntax should be restored, OR if the new OpenJK behaviour of breaking the parsing could be improved by adding warnings, OR any another solution that someone thinks is better.

cc @SzicoVII

[ensiform]

So long as you fix it by still keeping the optimized code, I don't care.

[dionrhys]

I've narrowed down the problem to Shader_CompressBracedSection in OpenJK. It allows shaders to have names with newlines in them and it parses the shader above as "#################### #################### textures/atlantica/atlantica_softclip" instead of bailing when a name has a newline in it.

The best way to fix this would probably be to skip a name when a newline is found, give a warning, then try to parse a name again (until it finds a starting brace for the shader).

[SzicoVII]

Given that you guys want to keep "backward" compatibility, I think the shader parsing should remain able to deal with such lines.

Also,just noticed that shader syntax errors that have too many }} cause OpenJK to crash on startup. I think in baseJA it just messed up the shaders ingame. Perhaps itd be nice to add a warning to the crash log so people would know what had caused it?

[ensiform]

Whether it should "deal with them" is irrelevant, it isn't simply dealing with them or treating them as comments, just seems to know that it isn't a shader because it gobbles it all into 1 giant buffer which somehow still works.

That crash shouldn't happen either and is probably tied to also having ###s in the file which is causing the parser to break as it is.

And there isn't a crashlog because you are running a "Final" build. aka no debug symbols.

[SzicoVII]

Huh? If you want to maintain backward compatibility, OpenJK should be able to read and use (properly) any shader files that baseJA can. Or have you ditched that goal?

As for the crash, that has nothing to do with # or Atlantica's shader, just any extra { or } in any shader file. And it appears to be fixed in the latest revision I downloaded so you can ignore that part now - sorry.

[eezstreet]

You shouldn't be using ### period, so...

Sent from my Windows Phone

---

From: SzicoVIImailto:notifications@github.com Sent: ‎5/‎22/‎2013 11:15 AM To: Razish/OpenJKmailto:OpenJK@noreply.github.com Cc: eezstreetmailto:eezstreet@live.com Subject: Re: [OpenJK] Broken shaders on Atlantica map (#271)

Huh? If you want to maintain backward compatibility, OpenJK should be able to read and use (properly) any shader files that baseJA can. Or have you ditched that goal?

As for the crash, that has nothing to do with # or Atlantica's shader, just any extra { or } in any shader file. And it appears to be fixed in the latest revision I downloaded so you can ignore that part now - sorry.

---

Reply to this email directly or view it on GitHub: https://github.com/Razish/OpenJK/issues/271#issuecomment-18285388

[xycaleth]

The point is that even if JKA wasn't meant to accept comments like that, it's what's expected from a user point of view based on past experience. And anyway, what's wrong with supporting #s in shaders? We don't fix poking/wiggling in the saber system, even though it's considered an exploit because it's what's expected. Same principal here.

And anyway, it shouldn't be crashing either way :p

[SzicoVII]

^^What he said.

[eezstreet]

I'm not really saying there's anything wrong with that. But I think it's ridiculous that we need to accept input from files which use black magic/bugs in order to work correctly. The system was never designed to allow for ## comments.

Sent from my Windows Phone

---

From: Alex Lomailto:notifications@github.com Sent: ‎5/‎22/‎2013 12:05 PM To: Razish/OpenJKmailto:OpenJK@noreply.github.com Cc: eezstreetmailto:eezstreet@live.com Subject: Re: [OpenJK] Broken shaders on Atlantica map (#271)

The point is that even if JKA wasn't meant to except comments like that, it's what's expected from a user point of view based on past experience. And anyway, what's wrong with supporting #s in shaders?

---

Reply to this email directly or view it on GitHub: https://github.com/Razish/OpenJK/issues/271#issuecomment-18288905

[SzicoVII]

Well if it doesn't read them then it ceases to be backward-compatible really doesn't it? I'm sure there are other mods that use that kind of syntax since I'm pretty sure that's where I decided to lay it out like that came from.

[mrwonko]

Well if it doesn't read them then it ceases to be backward-compatible really doesn't it?

Technically, yes. Personally I'd be fine with adding # for comments, but in general I don't think obvious bugs have to be kept for compatibility. When/if I do my refactoring project, I'm certainly not going to investigate what bugs could be exploited and keep those.

[ensiform]

@SzicoVII just out of curiosity where did you come up with the idea that ###s were comments? It's been pretty well known that C/C++ style // and /* ... */ have been the valid comment style in q3 engine for years shaders not excluded.

We aren't actively trying to give reasons its not going to be fixed to "work". Just trying to look for a better fix other than allowing just ###s as comments.

@xycaleth thats a pretty piss poor comparison tbh. And I HAD fixed it that way by allowing them to be comments until Didz wanted to investigate further.

[shinyquagsire23]

@ensiform Actually, several scripting languages and configuration formats use #'s as comments, including YAML, Bash, Perl, Python, Ruby, and many configuration formats (like those on Linux), so some developers may not know that the Q3A shaders don't accept these style of comments. If you're not going to allow them, at least throw some sort of visible error like

ERROR! Improper formatting in shader <insert path here>!

and then continue parsing so that any modders will be notified while testing or creating their maps and shaders and will fix them. It's a bit of a win-win. Players can still play, but they'll get an annoying error, and map developers will either get pestered by players or by the error and fix it.

[ensiform]

Yea go ahead and write that into it without breaking new parser.

[dionrhys]

I'll fix this when I have time with something similar to what @shinyquagsire23 suggested. The new shader code is actually broken so I'll be killing two birds with one stone.

[ensiform]

I would still prefer to have the syntax checking. Which his shaders are failing at. @dionrhys poke

[Razish]

Should revert shader parser for milestone 1, or at-least have it optional. Can look into an optimised parser with better error checking later.

does not follow the specification and the shaders failing should not be considered a bug.

[ensiform]

He fixed atlantica, theres probably other maps which use it though.

[Rick12345]

Most other custom maps use it, such as Taris RP, Ord mantell RP and so on and they're bugged, so it isn't simply Scizo's maps. It's most maps in JKA. I've tried removing the # from the shader txt file and it hasn't changed anything, it still displays broken textures so it doesn't seem like it's an easily fixed issue, client side that is.

[Razish]

It's most maps in JKA

Not in my experience - I've never come across it in the custom maps I play. It must be common in RP maps, though.

If removing the '#' characters didn't fix it, then it's likely a separate issue.

[ensiform]

Links to the maps?

[ensiform]

Ord Mantell works just fine if you replace the shaders properly. Sounds like you didn't replace it in the pk3 in the right place, or its used in another pk3 that still overrides it.

[Rick12345]

It's in the shader folder within the pk3 isn't it? That's where I attempted removing it, twice, even redid my entire base folder, still no luck.

[ensiform]

I can upload mine later, if you still have issues then. Then you have another pk3 that has priority over it with the shader that is broken still.

[ensiform]

https://dl.dropboxusercontent.com/u/13001904/Ord_Mantell_Rp.pk3

It was mantell.shader that had several instances of ######### for this map.

I get no issues using that pk3, but I have no other RP maps installed currently.

[Rick12345]

Still seems like a waste of effort to do that on like a hundred maps when I could either just wait on an eventual fix and acceptance of the ##### in the code and just play regular JAMP until then.

[ensiform]

It isn't a comment by any of the parsers old or new, the files are broken :frowning:

Invalid syntax will be treated as invalid syntax. But the newer parser is flawed in many other ways as it is so we will do something when we get to it. The RP community seems to be the general only area who decided to pass those bad shaders around besides @SzicoVII.

[ensiform]

I'm probably going to switch master branch to the Shader parsing that is in rend2. Which will still break on your maps but it at least has warning messages about it being incorrect and then maybe go from there to try and make it workable.

[DarthFutuza]

Was this ever resolved? If not, I'll just add my input that shinyquagsire23 suggestion seems best as far as how to treat #### based comments. (Console error, but ignore and compile anyway).

[ensiform]

It's been reverted to ioquake3 behavior which will actually show the warnings. (As in, its not broken the shaders are) where as the old code that was added to OpenJK was actually broken and having some crash issues as well.

At least it should push active mappers to fix their maps.

Unfortunately it doesn't really work in such a way that he described. Its not processed or parsed to a point where it can behave that way at this point.

As it is now, it does syntax checks to check for the structure of a file so that one bad file does not fuck up the rest in terms of incorrectly placed braces. This is done during the initial load before any of the so-called parsing happens before it puts the whole shader buffer into the mega buffer.

FWI other than RP maps, there are very few who this affects. Since he fixed atlantica ages ago.