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
tr_shader: revamp the stage collapse code #184
Conversation
illwieckz
commented
Mar 17, 2019
•
edited
edited
- improve readability
- reduce cpu complexity
- make it extendable
- fix bug when surface with reflectionmap is not normalmapped
- fix lightmap being applied twice and glow map being lightmapped (note: lightmap alphaGen blend not yet supported)
- reduce the lighting stage type count to two: Phong (any combination including diffuse and something that is not material, including specular) and PBR (any combination including diffuse and material)
- fix the bug when a reflection type is applied to shader lighting type
2ac5ad6
to
8ecc7af
Compare
8ecc7af
to
0c5f6ad
Compare
You're confident that shader stages should never have any order dependency and can be arbitrarily shuffled? The old version only merges adjacent stages, while the proposed one merges them from anywhere. |
You put the feet on it. Basically, the initial quake3 material stages were order dependent. And there was no thing like a glow map, you just write a stage with a color map, then a stage that blend lightmap, then a stage that blend a color map. First color map is a diffuse map because you written it before lightmap, and second color map is a glow map because you written it after. Once you use stage keywords like Basically you set a diffuseMap file, a specularMap file, a normalMap file, a glowMap file, optionally a lightmap stage to configure the lightMap stage, and the engine knows what to do and in which order. A glow map rendered before a diffuse map is wrong (diffuse map will be painted over and hide it), a glow map rendered before a light map stage is wrong (shadows will be painted over the glow map, but the purpose of a glow map is to not be affected by shadows), a light map before a diffuse map is wrong (diffuse map with be painted over and hide it). The only situation were order would be required, is that if we were able to load two diffuseMap stage, or two specularStage, or two normalMap stage, or two glowMap stage, for example to blend two glowMap stages as one. I really doubt the engine supports such things, as the code seems to send one diffuse/specular/normal/glow map per pixel to glsl shaders. If I'm right the material language is able to load as map the result of another material, this another material may be able to blend. I'm not sure if this work, but that would be the correct way to merge glow maps for example. Basically the Part of this PR is to ensure stages are reordered the way the engine expects them, that's the only way to fix some bugs in any way. So, if someone can prove me that we can put two diffuseMap stage in a material, please talk, it looks to have been designed against that. :-) |
0c5f6ad
to
67f2048
Compare
note that this is still ordered: the existing stages that are not collapsed together are still before/after the collapsed one. The question is: does it makes sense to set multiple time the same stage: for diffuse/normal/specular/light/glow: it does not make sense. We seen in the code that reflection map may be blended with a normal map, and diffuse map may be blended with a normal map, current code assumes it's the same, perhaps it's wrong and we can set two normal maps, one for reflection, one for diffuse, in that case I would look at it, and that would be the only exception. |
Also one note: the heightmap in normalMap stage must be applied on both diffuse, specular and glow maps, there is no such things as ordering in that process (and there must not be). |
Can we get a warning if the shader contains more than one of any of these kinds of stage? |
That's doable, the original code was already looking for only one kind of these stage and ignore others once first was found. |
b906fca
to
abd743c
Compare
I added the warning. I also reduced the light stage merge to just a disablement (as there is already an implicit lightmap stage within diffuse stage), the problem is that I don't know how to pass the blend option. So it fixes like 90% (or more) of the double-lightmap and lightmap-over-glowmap issues. To fix the remaining ones (if they exist), more underlying design may have to be done and that would be better to do it in another PR. The current disablement fixes many bugs but does not add some, it just lefts some. |
abd743c
to
4dd00a9
Compare
I have to figure out if there is more than one way to blend lightmap. Perhaps not, perhaps all the rare variations I see are just mistake or equivalences. If I look at the 924 lightmap stages in xonotic maps, they all (but some) use the same stage:
There is 4 differences in
is a mistake that mixes two stage in one, that must never happen. The Then there if I remove the faulty erbium ones, I fond 17 lightmap stage that use a This is an example of them (in
The Then the other example is this one (in
It's a very similar shader to the water one, with a color map and a blended reflection map. I don't know what does Then we have this (in
I don't know what it does, there is only two of them. There is one similar (but a bit different) in Then in
If those variations are legit I have a prototype that disables the implicit lightmap stage within diffuse stage and sort the stages the way an eventual diffuse stage is always before the lightmap stage and an eventual glow stage is always after the lightmap stage. With proper tests this would only match on those custom shaders and 90% of them will use the implicit lightmap stage of the diffuse one which is very good for performances. The real question is:are those custom lightmap stage legits? Adding those tests and the resorting is adding much noise to the code, but is doable. On a side note that the terrain blending shader uses two color maps, no diffuse stage. It's the only supported way at this time and we hence don't support normalmap, parallax and other feature on such shader. We also have a lot of those variants, much more than xonotic: 161 / 1222 (305 custom lightmaps + 917 diffuse maps), with
or in
Or this in
Or this two others in
I also found this on
So, we can clearly see patterns for some various usages and that looks legit. Our shaders seems to make the effort to not use diffuseMap with those custom lightmaps, that's also why our maps are less likely than xonotic ones to help us to make dæmon better because they already avoid the traps. It means we don't support normal/specular/parallax on shaders that need custom lightmaps while DarkPlaces seems to. |
It looks like the common lightmap stage is:
For some reason the default Edit: there is a special code to set to Daemon/src/engine/renderer/tr_shader.cpp Lines 2509 to 2520 in e920d3a
So it looks like only “filter” as to be tested. |
4dd00a9
to
d81aed6
Compare
So I added test for custom lightmap and then makes light stage and glow stage standalone and ordered after collapsed diffuse stage. Since most of the light stages are common ones and since most of custom stages comes with stuff like |
Note that I don't like this code at all and I would prefer to see the lightmap blending being done in the diffuse collapsed stage instead. |
d0cacf9
to
3476f84
Compare
After some deep thoughts I think that a lot of the Basically if that is found:
It would be interpreted as:
It would not only obsolete the collapse computation, it would make possible to do one day a terrain shader this way:
But this is not for today, let's be iterative and the current code is good enough even if I dream of something better. In any way the glsl code is not yet ready for such dream. 😁 |
To illustrate the double lightmap and overpainted glow map in Unvanquised, you can look at the example in #186 These is another example with a xonotic map, how it looks when glow map is explicitely disabled (there is double lightmap over diffuse maps): This is how it looks with glow map enabled, it get repainted with the second lightmap (there is double lightmap, before and after glow map): This is now with fixed collapse code, with glow map enabled (one lightmap before glow map): This is an example of problem with previous stage collapsing code, this texture has both diffuse, normal, specular, height and reflection map, you'll notice that only the reflection map with the glassy holes is correctly applying reflection map itself, the normalmap and the heightmap. The other collapse stage (diffuse, normal, specular) is disabled for the solid part of that same shader: This is how it looks when fixed: Also notice how the whole map is also more brighter because of the double-lightmap being fixed. Note that I don't exclude that the glassy holes may be a bug themselves, I don't see them in xonotic (but their materials have a lot of mistakes, and perhaps the cubemap reflection parallax feature is not in DarkPlaces), but at least it produces what the material says and we can confirm that reflection maps get properly normalmapped and parallaxed. |
- avoid double shadowing - avoid to paint shadow over glowmap
if a custome light stage is found, - do not collapse light stage with diffuse stage - do not collapse glow stage with diffuse stage - ensure diffuse < light < glow
3476f84
to
650bb3f
Compare