Remove all bump mapping and parallax occlusion related code. #10487

lhofhansl · 2020-10-12T20:31:20Z

See #10479 for discussion.

This PR removes bump mapping, parallax occlusion, and normal mapping code, since it was buggy and not fully implemented.
This simplifies the shader code and hopefully makes it easier to start over.

Note that this also remove the normal texture feature - since it normal textures are no longer used for anything.

paramat · 2020-10-12T21:18:18Z

I can review and test this, although i feel a little underqualified in some areas of the changes =)

lhofhansl · 2020-10-12T21:59:39Z

@HybridDog FYI since you've worked in this area.

builtin/settingtypes.txt

paramat · 2020-10-12T22:29:42Z

Interesting how the documentation reveals these shaders are actually 'experimental', unfinished and buggy. It is more of a mess than i realised.

Bumpmapping and parallax occlusion are currently experimental features:
Parallax occlusion with relief-mapping mode does not yet work correctly together with Minetest's Fastfaces.

Oddly, relief mapping mode is actually set as the default mode, and fastfaces are everywhere.

src/client/mesh_generator_thread.cpp

paramat · 2020-10-12T23:58:59Z

I will make this my top priority for review.

lhofhansl · 2020-10-13T00:19:31Z

@paramat Thanks! You already caught two issues - I've been sloppy.

lhofhansl · 2020-10-13T19:58:35Z

~~@TheTermos does not feel that we should remove the parallax occlusion code.~~

Sorry, that was @HybridDog .

TheTermos · 2020-10-13T21:50:38Z

@TheTermos does not feel that we should remove the parallax occlusion code.

Where did that come from?

lhofhansl · 2020-10-13T22:52:33Z

@TheTermos Oops. That was @HybridDog. I truly apologize!

paramat · 2020-10-14T20:10:16Z

My testing in the issue which shows pixel wrapping reminded me of something i thought i would mention.

The wrapping of pixels from the opposite edge, issue #2815 , was fixed in 655fc60 by adding the tiledef fields 'tileable_vertical', 'tileable_horizontal'.

Perhaps we should leave these tiledef fields present in code, for now at least, as they are likely to be needed by any new implementation of bumpmapping / parallax occlusion. ~~However perhaps we should remove mention of them in lua_api.txt as they will be irrelevant?~~ EDIT: Done.

We might decide to remove these tiledef fields, but i suggest that we consider and do that in a later PR, to not bloat and delay this PR.

lhofhansl · 2020-10-15T07:29:58Z

Thanks @paramat, I don't see that I'm removing any tiledef fields in this PR. Happy to shrink the scope of this, which part are you referring to?

paramat · 2020-10-15T15:09:10Z

I don't see that I'm removing any tiledef fields in this PR

Sorry to be unclear, i do not mean to imply you are removing those in this PR, you are not. I am just mentioning the related tilespec code and suggesting we leave that in, for now at least.
~~So the only change to the PR i am suggesting is removing the documentation of 'tileable_vertical/horizontal' from lua_api.txt.~~ EDIT: Done.

client/shaders/object_shader/opengl_fragment.glsl

paramat · 2020-10-15T17:18:50Z

In client/shaders/object_shader/opengl_fragment.glsl, do the following need removing?
EDIT: Resolved.

uniform sampler2D normalTexture;
uniform sampler2D textureFlags;

uniform vec3 eyePosition;

varying vec3 vNormal;

bool normalTexturePresent = false;
bool texTileableHorizontal = false;
bool texTileableVertical = false;
bool texSeamless = false;

void get_texture_flags()
{
	vec4 flags = texture2D(textureFlags, vec2(0.0, 0.0));
	if (flags.r > 0.5) {
		normalTexturePresent = true;
	}
	if (flags.g > 0.5) {
		texTileableHorizontal = true;
	}
	if (flags.b > 0.5) {
		texTileableVertical = true;
	}
	if (texTileableHorizontal && texTileableVertical) {
		texSeamless = true;
	}
}
vec4 get_normal_map(vec2 uv)
{
	vec4 bump = texture2D(normalTexture, uv).rgba;
	bump.xyz = normalize(bump.xyz * 2.0 - 1.0);
	return bump;
}

In void main(void):

	vec4 bump;

paramat · 2020-10-15T17:23:55Z

In client/shaders/nodes_shader/opengl_fragment.glsl, do the following need removing?
EDIT: Resolved.

uniform sampler2D normalTexture;
uniform sampler2D textureFlags;

uniform vec3 eyePosition;

paramat · 2020-10-15T17:28:57Z

In client/shaders/nodes_shader/opengl_vertex.glsl, does the following need removing?
EDIT: Resolved.

uniform vec3 eyePosition;

paramat · 2020-10-15T17:59:05Z

I tested. Main menu is fine and everything seems to work ok.
Reviewed code and all looks fine, apart from the extra removals needed from shader files detailed above.
Will retest after those (and lua_api.txt) are attended to and hopefully will then approve.

lhofhansl · 2020-10-15T18:42:49Z

Arrgghh. Thanks @paramat . I'll do a second pass through what can be removed and I'll remove the tilespec doc.

I wasn't sure if I wanted to remove all the uniforms from the shaders. I'll double check these.

paramat · 2020-10-15T20:58:36Z

I wasn't sure if I wanted to remove all the uniforms from the shaders

Ok i see. Although it will all still be in history to refer too, so not sure if there is advantage to leaving some stuff in creating a confusing mess.

I remember that 'vertex tangent' stuff was added by RBA purely for bumpmapping and parallax occlusion, in this commit 43fcfbf
Although at that point in history vertex tangents were always used, which caused a controversy because performance was greatly reduced. Later, vertex tangents were only used when shaders were enabled and when bumpmapping or parallax occlusion was enabled.

I did a search for 'tangents' in current code and found some extra stuff not in your PR:

minetest/src/client/mesh.cpp

Line 381 in 9bba52c

case video::EVT_TANGENTS: {

minetest/src/client/mesh.cpp

Line 978 in 9bba52c

case video::EVT_TANGENTS:

lhofhansl · 2020-10-15T22:40:47Z

I feel the following needs to be left in:

case video::EVT_TANGENTS: since this is a valid enum member
vNormal, since this is generally useful for the fragment shader to have.
eyePosition, as this is also generally good to have

lhofhansl · 2020-10-15T23:07:44Z

The failure on Android appears unrelated.

paramat · 2020-10-16T18:30:48Z

I feel the following needs to be left in:

That seems fine to me. I think this has a good balance of how much to remove.
Also i think we should not get too perfectionist for this first PR, as long as everything works, more could be cleaned up in a follow up PR.
I will retest, rereview and hopefully approve as soon as possible.

paramat · 2020-10-16T20:18:06Z

A related warning during compilation:

/src/client/game.cpp: In member function ‘virtual void GameGlobalShaderConstantSetter::onSetConstants(irr::video::IMaterialRendererServices*, bool)’:
/src/client/game.cpp:553:5: warning: unused variable ‘normal_tex’ [-Wunused-variable]
     normal_tex = 1,
     ^~~~~~~~~~
/src/client/game.cpp:554:5: warning: unused variable ‘flags_tex’ [-Wunused-variable]
     flags_tex = 2;
     ^~~~~~~~~

paramat · 2020-10-16T20:27:29Z

All my previous comments are attended to, rereviewed code, retested, all seems fine.
~~Once you fix the warnings and retest you can assume my approval and merge.~~

lhofhansl · 2020-10-17T02:54:25Z

I saw that warning too, but I did not want remove that enum. Hmm... I could.
Also, since the Android failure is persistent and I do not see this with other PRs perhaps it is pertinent?

paramat · 2020-10-17T15:45:19Z

FAILURE: Build failed with an exception.
Task :native:downloadSqlite FAILED
Download https://www.sqlite.org/2020/sqlite-amalgamation-3320200.zip
What went wrong:
Execution failed for task ':native:downloadSqlite'.
Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
de.undercouch.gradle.tasks.download.org.apache.http.conn.HttpHostConnectException: Connect to www.sqlite.org:443 [www.sqlite.org/2600:3c00:0:0:f03c:91ff:fe96:b959] failed: Network is unreachable (connect failed)

It looks like a connection failure unrelated to these changes, so being persistent and this PR only seems odd. Maybe @rubenwardy could advise?

The compilation warnings are not a problem so consider my approval given, although we might want to get some input on that build failure before merging.

rubenwardy · 2020-10-17T15:55:17Z

That's a temporary error due to website downtime, I've restarted the build

paramat · 2020-10-17T16:06:44Z

Good to know, and thanks.
So i think this should be merged if builds are successful, before conflicts start arising and so i do not have to rerereview. We can always do some additional clean up in a follow up PR.

paramat · 2020-10-17T17:24:39Z

Can be merged now.

lhofhansl · 2020-10-17T19:36:16Z

Actually lemme look at that enum. If it can be safely removed I'll do that and then merge. Otherwise I'll merge as is.

lhofhansl · 2020-10-17T19:45:24Z

It's not an enum anyway, I just removed he unused variables, which avoids that warning, going to merge soon.

paramat · 2020-10-17T23:18:58Z

Ok looks fine.

lhofhansl requested a review from paramat October 12, 2020 20:31

lhofhansl added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label Oct 12, 2020

paramat added the Shaders label Oct 12, 2020

paramat mentioned this pull request Oct 12, 2020

Normalmaps: Apply only specific texture modifiers #7144

Closed

paramat reviewed Oct 12, 2020

View reviewed changes

builtin/settingtypes.txt Outdated Show resolved Hide resolved

paramat reviewed Oct 12, 2020

View reviewed changes

src/client/mesh_generator_thread.cpp Show resolved Hide resolved

paramat reviewed Oct 15, 2020

View reviewed changes

client/shaders/object_shader/opengl_fragment.glsl Outdated Show resolved Hide resolved

paramat added the One approval ✅ ◻️ label Oct 16, 2020

Remove all bump mapping and parallax occlusion related code.

5b8ccf8

lhofhansl merged commit ed22260 into minetest:master Oct 17, 2020

This was referenced Oct 17, 2020

Discuss: Remove all Bump Mapping and Parallax Occlusion code #10479

Closed

Shaders for Android (GLES 2) #10506

Merged

SmallJoker mentioned this pull request Jan 22, 2021

Some shaders are missing #10848

Closed

hecktest mentioned this pull request Jul 26, 2021

Multipass shadow mapping #11470

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove all bump mapping and parallax occlusion related code. #10487

Remove all bump mapping and parallax occlusion related code. #10487

lhofhansl commented Oct 12, 2020 •

edited

Loading

paramat commented Oct 12, 2020

lhofhansl commented Oct 12, 2020

paramat commented Oct 12, 2020 •

edited

Loading

paramat commented Oct 12, 2020

lhofhansl commented Oct 13, 2020

lhofhansl commented Oct 13, 2020 •

edited

Loading

TheTermos commented Oct 13, 2020

lhofhansl commented Oct 13, 2020

paramat commented Oct 14, 2020 •

edited

Loading

lhofhansl commented Oct 15, 2020

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

lhofhansl commented Oct 15, 2020

paramat commented Oct 15, 2020 •

edited

Loading

lhofhansl commented Oct 15, 2020

lhofhansl commented Oct 15, 2020

paramat commented Oct 16, 2020

paramat commented Oct 16, 2020

paramat commented Oct 16, 2020 •

edited

Loading

lhofhansl commented Oct 17, 2020

paramat commented Oct 17, 2020 •

edited

Loading

rubenwardy commented Oct 17, 2020

paramat commented Oct 17, 2020

paramat commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

paramat commented Oct 17, 2020

Remove all bump mapping and parallax occlusion related code. #10487

Remove all bump mapping and parallax occlusion related code. #10487

Conversation

lhofhansl commented Oct 12, 2020 • edited Loading

paramat commented Oct 12, 2020

lhofhansl commented Oct 12, 2020

paramat commented Oct 12, 2020 • edited Loading

paramat commented Oct 12, 2020

lhofhansl commented Oct 13, 2020

lhofhansl commented Oct 13, 2020 • edited Loading

TheTermos commented Oct 13, 2020

lhofhansl commented Oct 13, 2020

paramat commented Oct 14, 2020 • edited Loading

lhofhansl commented Oct 15, 2020

paramat commented Oct 15, 2020 • edited Loading

paramat commented Oct 15, 2020 • edited Loading

paramat commented Oct 15, 2020 • edited Loading

paramat commented Oct 15, 2020 • edited Loading

paramat commented Oct 15, 2020 • edited Loading

lhofhansl commented Oct 15, 2020

paramat commented Oct 15, 2020 • edited Loading

lhofhansl commented Oct 15, 2020

lhofhansl commented Oct 15, 2020

paramat commented Oct 16, 2020

paramat commented Oct 16, 2020

paramat commented Oct 16, 2020 • edited Loading

lhofhansl commented Oct 17, 2020

paramat commented Oct 17, 2020 • edited Loading

rubenwardy commented Oct 17, 2020

paramat commented Oct 17, 2020

paramat commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

lhofhansl commented Oct 17, 2020

paramat commented Oct 17, 2020

lhofhansl commented Oct 12, 2020 •

edited

Loading

paramat commented Oct 12, 2020 •

edited

Loading

lhofhansl commented Oct 13, 2020 •

edited

Loading

paramat commented Oct 14, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 15, 2020 •

edited

Loading

paramat commented Oct 16, 2020 •

edited

Loading

paramat commented Oct 17, 2020 •

edited

Loading