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

refactor and fix normalmap, parallax, reflectionmap, glowmap, GL/DX normal switch #180

Open
wants to merge 11 commits into
base: master
from

Conversation

3 participants
@illwieckz
Copy link
Member

illwieckz commented Mar 10, 2019

This PR first refactors some normalmap code to avoid it to be copypasted in 6 files, and to be sure the same features are supported anywhere, and that was not true since r_NormalScale was not used anytime.

Then it fixes the normalmap code by reusing a simpler code from learnopengl.com.

This code is so better that it not only reduced the number of lines and removed expensive computations like sqrt or pov, it also fixed some graphical bugs (will write more on this later) and made the code able to revert a normalmap at the fingertip of a multiplication by -1, which should have been true before but was not.

Then, the PR adds a keyword shader to allow people to revert a normalmap that is stored the wrong way, without changing the file itself.

Then the PR auto enables the DirectX format for DarkPlaces detected normalmaps.

So, this code not only fixed bugs and is not only more readable, it also enables engine to load a xonotic map after an unvanquished map in a totally transparent manner.

The PR also refactors parallax code and fixes the orientation.

Reflection maps are now loaded as cubemap (that was a bug), a new kind of cubemap naming scheme is added (for compatibility with Darkplaces).

The parallax keyword can now receive the same arguments dpoffsetmapping expects.

Parallax reflection cubemap is now implemented.

Shader stage collapse was revamped with a lot more simpler code, and less buggy one.

It allowed to merge the implicit lightmap that comes with diffusemap and the legacy explicit ones, to avoid double shadowing and shadows being painted over glow maps.

A bug is also fixed when reflection_CB stage type was set to shader when the shader stage type is only used for lighting type (DBS or PBR).

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch 2 times, most recently from 4df4061 to 47101ac Mar 10, 2019

@illwieckz illwieckz changed the title WIP: refactor and fix normalmap, implement OpenGL/DirectX normalmap format switch refactor and fix normalmap, implement OpenGL/DirectX normalmap format switch Mar 10, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 47101ac to 6bbd749 Mar 13, 2019

@illwieckz illwieckz changed the title refactor and fix normalmap, implement OpenGL/DirectX normalmap format switch refactor and fix normalmap and parallax, implement GL/DX normal switch Mar 13, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 13, 2019

I also refactored parallax code and fixed orientation the way it seems right to do (transforming [1-0] into [0-1] at the pixel level).

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 6bbd749 to 051888f Mar 13, 2019

@illwieckz illwieckz added this to To do in Unvanquished via automation Mar 13, 2019

@illwieckz illwieckz added this to To do in Xonotic via automation Mar 13, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch 2 times, most recently from c584b6e to 06a71aa Mar 13, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 13, 2019

I added a commit that implements dpoffsetmapping. This is a very useful keyword allowing to scale and offset an heightmap on a per shader basis.

See the texture in foreground, the edge is not correct because the heightmap provides height from the bottom instead of depth from the top, hence the whole texture is displayed lower than the surface itself:

dpoffsetmapping

The same texture once dpoffsetmapping is implemented:

dpoffsetmapping

Some more screenshots, light mapping on edges:

dpoffsetmapping

Realtime light mapping on edges:

dpoffsetmapping

OK from the dretch point of view floor with 0.02 depth scale.
I recommend to switch from 0.03 to 0.02 since we never used 0.03 yet and 0.02 produces less artifacts when seen from dretch point of view and it is Darkplaces default, hence 0.02 is a better choice for both Unvanquished and Xonotic:

dpoffsetmapping

Note that the bricks on the wall are flats because those textures use loose heightmap (and darkplaces computes normalmap from heightmap), something we don't do yet, otherwise everything else is right, notice there is no dark blotches in grooves anymore:

dpoffsetmapping

Some more coolness:

dpoffsetmapping

Note that I'm adding engine code to GPL files or sometime rewriting code but I prefer to contribute it under BSD , perhaps I will add some comments to identify what is more permissive. The normal map code is brand new, the dpoffsetmapping code does not copypaste DarkPlaces (and the way our engine is designed is not very compatible with the way it is implemented in Darkplaces).

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from f0fd80c to e4c9bc7 Mar 13, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 13, 2019

I discovered that our original code was not applying parallax offset to glowmaps, this is now fixed.

Before:
dpoffsetmapping

After:
dpoffsetmapping

Note tat the displayed texture has a buggy parallax on Dæmon but is also buggy in Darkplaces (the artifacts probably just differ because of implementation details).

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from e4c9bc7 to 1419ca6 Mar 13, 2019

@DolceTriade

This comment has been minimized.

Copy link
Contributor

DolceTriade commented Mar 15, 2019

Very cool! @gimhael if you get a chance, could you review?

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 71a5707 to 15e5c39 Mar 16, 2019

@illwieckz illwieckz changed the title refactor and fix normalmap and parallax, implement GL/DX normal switch refactor and fix normalmap, parallax, reflectionmap, glowmap, implement GL/DX normal switch Mar 16, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 15e5c39 to 553d3d9 Mar 16, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 16, 2019

So, more awesome stuff:

Reflection maps are now loaded as cubemap (that was a bug), a new kind of cubemap naming scheme is added (for compatibility with Darkplaces).

The parallax keyword can now receive the same arguments dpoffsetmapping expects.

Parallax reflection cubemap is now implemented.

Shader stage collapse was revamped with a lot more simpler code, and less buggy one.

It allowed to merge the implicit lightmap that comes with diffusemap and the legacy explicit ones, to avoid double shadowing and shadows being painted over glow maps.

A bug is also fixed when reflection_CB stage type was set to shader when the shader stage type is only used for lighting type (DBS or PBR).

@illwieckz illwieckz changed the title refactor and fix normalmap, parallax, reflectionmap, glowmap, implement GL/DX normal switch refactor and fix normalmap, parallax, reflectionmap, glowmap, GL/DX normal switch Mar 16, 2019

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch 3 times, most recently from 8b0d2e7 to d199763 Mar 16, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 17, 2019

OK, people must know about the fact that ALL Unvanquished maps are broken with this branch because I fixed a bug that was hiding another.

Basically, all our normal maps are compressed with crunch -dxn and our engine loads them with reverted blue channel (Z component). This was hidden by the bug in the normalmap code that was loading the Z component from alpha channel instead of blue one. Hopefully we did not ship any heightmap in alpha channel and since crunch -dxn drops the alpha channel in any way, it looks like the engine was just using an empty Z component). The normalmap loads correctly if loaded from another format (like webp).

The current tree is meant to be correct. I created a test map with assets I created to be sure they are the way they must be, then fixed the engine to render the test map. The correct engine in this tree is able to render correctly xonotic maps and unvanquished maps if their normal maps were not compressed with crunch -dxn. I'll write an issue about this soon, but we need someone to fix the normal map loading when they are compressed with dxn (use this branch as a base as master is not correct).

The symptom is a map being almost entirely dark. No need for special map to reproduce the bug, just load released chasm with this branch (or any other map).

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 17, 2019

It's too hard to review 27 commits. Can you split this up into multiple PRs?

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from d199763 to 90878bd Mar 17, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 17, 2019

I reduced the number of commits to 10 by:

  • reordering and squashing commits
  • merging one that is very trivial: 178bb54
  • moving stage collapse commits to another PR: #184
  • retaining some bikeshedding on a local branch for later

note that the reflection map parallax does not work until #184 is merged

The dpreflectcube and later commit may be another PR but they require those from this branch to compile, so I left them.

{ "rt", "lf", "bk", "ft", "up", "dn" },
{ true, true, false, true, true, false },
{ false, false, true, false, false, true },
{ 90, -90, 0, 0, 90, -90 },
},
{
"DarkPlaces",

This comment has been minimized.

@illwieckz

illwieckz Mar 17, 2019

Author Member

I had no idea to name this, Xonotic use some OpenGL-like skyboxes without _ separator. I may name it “OpenGL” too.

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 90878bd to 62fd8d0 Mar 17, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 17, 2019

Note that second commit, the one named “glsl/normal: fix code to compute normal in tangent space with simpler code” is the one that makes dæmon unable to lit our maps due to the normalmap not being properly loaded from dxn crunch files. But this commit is needed as it fixes rendering bugs and is a required foundation for many other fixes.

#183 is a blocking issue.

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch 2 times, most recently from a9d5055 to 0297109 Mar 17, 2019


#if defined(r_NormalScale)
N.z *= r_NormalScale;
normalize(N);

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

does this line have any effect?

This comment has been minimized.

@illwieckz

illwieckz Mar 18, 2019

Author Member

the normalize one? I really don't know

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

I don't think it does. If you don't know, where are you getting the code from?

This comment has been minimized.

@illwieckz

illwieckz Mar 18, 2019

Author Member

copy pasted from our code, this is a refactor, some of the normal code in other files had this.

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

Let's just delete this r_NormalScale stuff. The code does not make sense: (1) Why would you want to multiply just one component of the vector by some value? (2) According to any docs/examples I find, normalize is supposed to be used by the return value, not an in-place modification.

Nothing will be lost as it would only have an effect if you are playing with cvars.

Show resolved Hide resolved src/engine/renderer/glsl_source/reliefMapping_fp.glsl Outdated
@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 18, 2019

If some of the code makes everything totally broken it should be pushed to a branch other than master.

@illwieckz illwieckz force-pushed the illwieckz:fixnormalmap branch from 0297109 to bb20526 Mar 18, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 18, 2019

I restored the Capcom code to abuse DXT1/5 alpha channel to compress normal maps and I passed the heightMapInNormalMap boolean to glsl so the engine does not use the trick when it loads a normal map file with an height map in alpha channel.

I also added comments to tell what's happening…

This heightMapInNormalMap will be used in the future by parallax code to know from where computing the parallax when height map from loose file support will be added. Loading of height map from loose is required since DXn can't store the height map in alpha channel because of the alpha channel abuse.

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 18, 2019

I also fixed the compilation of the liquid related code so it builds if enabled. By the way I left it disabled and produces visual garbage in any way.

{
Log::Warn("unknown normalFormat keyword '%s' not supported in '%s'", token, shader.name );
SkipRestOfLine( text );
continue;

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

I think you want break here.

@@ -199,13 +201,37 @@ void computeDLights( vec3 P, vec3 N, vec3 I, vec4 diffuse, vec4 specular,
#endif
}

vec3 NormalFlip(vec3 N)

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

Nice hack :)

{
token = COM_ParseExt2( text, false );

if ( token[ 0 ] == 0 )

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

Since token[0] is a char it would be nicer to do !token[0] or token[0] == '\0' instead of comparing to an int.

This comment has been minimized.

@illwieckz

illwieckz Mar 18, 2019

Author Member

you're right, and I'm especially the kind of guy that likes explicit correct comparisons, I'll do that :-)

@@ -268,10 +279,10 @@ float RayIntersectDisplaceMap(vec2 dp, vec2 ds, sampler2D normalMap)
{
depth += size;

vec4 t = texture2D(normalMap, dp + ds * depth);
float t = 1.0 - texture2D(normalMap, texCoords + ds * depth).a;

This comment has been minimized.

@slipher

slipher Mar 18, 2019

Contributor

Why is the 'one minus' introduced?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.