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

Fix MSAA stripes #9247

Merged
merged 1 commit into from
Dec 4, 2020
Merged

Fix MSAA stripes #9247

merged 1 commit into from
Dec 4, 2020

Conversation

HybridDog
Copy link
Contributor

This only works when shaders are enabled.
The centroid varying avoids that the textures (which repeat themselves out of bounds) are sampled out of bounds in MSAA.
If MSAA (called FSAA in minetest) is disabled, the centroid keyword does nothing.

Related issue: #6860
An older related PR: #8123

@numberZero
Copy link
Contributor

  1. Does this work on older systems? (I mean those not supporting MSAA as well, they should work as before).

  2. Is there any noticeable performance penalty? As you wrote it, it applies to everything including even solid nodes; that might fix a more subtle bug I’ve seen some time ago, though.

  3. Does it work well together with mipmapping, anisotropic filtering, etc.?

The shading language specification considers derivatives derived from centroid varings to be so fraught with inaccuracy that it was resolved they are simply undefined.
 — https://www.opengl.org/pipeline/article/vol003_6/

P.S. MSAA (multisample anti-aliasing) is a kind of FSAA (full-scene anti-aliasing) used in Minetest.

@HybridDog
Copy link
Contributor Author

HybridDog commented Jan 5, 2020

Does this work on older systems?

It does nothing when shaders are disabled. varying centroid is explained in the glsl 1.2 (which is related to opengl 2.1) specification. The centroid keyword is simply ignored when MSAA is disabled.

Is there any noticeable performance penalty?

There should not be any penalty if MSAA is disabled.
In my opinion, MSAA with small nearest-neighbour sampled textures looks worse than disabled MSAA if centroid is not used.

As you wrote it, it applies to everything including even solid nodes; […]

I've only changed the nodes shader; maybe the wield shader should be changed, too.

Does it work well together with mipmapping, anisotropic filtering, etc.?

Something similar to the derivation is only required for parallax occlusion I think. Anyways, the wrong-border-colour artifacts are probably much more disturbing than artifacts caused by the centroid offset.

P.S. MSAA (multisample anti-aliasing) is a kind of FSAA (full-scene anti-aliasing) used in Minetest.

According to Wikipedia, FSAA is the same as SSAA: https://en.wikipedia.org/wiki/Spatial_anti-aliasing#Super_sampling_/_full-scene_anti-aliasing
Wikipedia has this information from Jason Gregory, Jeff Lander (2009). Game Engine Architecture. A K Peters, Ltd. p. 39. ISBN 978-1-56881-413-1.

@HybridDog HybridDog marked this pull request as ready for review January 12, 2020 14:00
@SmallJoker SmallJoker added Bugfix 🐛 PRs that fix a bug Rebase needed The PR needs to be rebased by its author. Shaders labels Jun 7, 2020
Copy link
Contributor

@hecktest hecktest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection except for maybe the unnecessary _bounded suffix, that's just noise and redundant with the centroid keyword. Maybe a comment could explain why it's centroid instead.

@HybridDog
Copy link
Contributor Author

Maybe a comment could explain why it's centroid instead.

Done. I hope the comment is not too long.

@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Jun 15, 2020
sfan5
sfan5 previously approved these changes Jun 15, 2020
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as describedNevermind.

@sfan5
Copy link
Member

sfan5 commented Jun 15, 2020

This causes pretty severe issues with mipmapping enabled on rails:
grafik

@HybridDog
Copy link
Contributor Author

This causes pretty severe issues with mipmapping enabled on rails

I don't see the problem. Mipmapping with partially transparent textures works wrong anyway.

MSAA disabled: I can't see a difference:

master:
master

this branch:
this_branch

MSAA enabled:

master:
master_msaa

this branch:
this_branch_msaa

@sfan5
Copy link
Member

sfan5 commented Jun 16, 2020

I don't see the problem. Mipmapping with partially transparent textures works wrong anyway.

Sure, but that doesn't mean it should look worse than before.

@HybridDog
Copy link
Contributor Author

HybridDog commented Jun 22, 2020

Sure, but that doesn't mean it should look worse than before.

In my opinion it is negligible in comparison to the visible MSAA stripes,
and the disappearing railway sleepers are much more noticeably than the mipmapping with centroid issue.

This only works when shaders are enabled.
The centroid varying avoids that the textures (which repeat themselves out of bounds) are sampled out of bounds in MSAA.
If MSAA (called FSAA in minetest) is disabled, the centroid keyword does nothing.
@HybridDog
Copy link
Contributor Author

rebased.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with OpenGL 4.6.0. Looks good, although I can barely find any difference.

@SmallJoker SmallJoker merged commit e73c5d4 into minetest:master Dec 4, 2020
@HybridDog HybridDog deleted the m_msaa_centroid branch December 4, 2020 19:43
@sfan5
Copy link
Member

sfan5 commented Dec 15, 2020

This breaks GLES (with shaders enabled):

2020-12-15 22:29:24: ERROR[Main]: Irrlicht: GLSL shader failed to compile
2020-12-15 22:29:24: ERROR[Main]: Irrlicht: 0:56(1): error: syntax error, unexpected NEW_IDENTIFIER, expecting end of file

[...]
2020-12-15 22:29:24: WARNING[Main]: 55: varying lowp vec4 varColor;
2020-12-15 22:29:24: WARNING[Main]: 56: centroid varying mediump vec2 varTexCoord;
2020-12-15 22:29:24: WARNING[Main]: 57: 
2020-12-15 22:29:24: WARNING[Main]: 58: varying vec3 eyeVec;
[...]

HybridDog added a commit to HybridDog/minetest that referenced this pull request Dec 17, 2020
This only works when shaders are enabled.
The centroid varying avoids that the textures (which repeat themselves out of bounds) are sampled out of bounds in MSAA.
If MSAA (called FSAA in minetest) is disabled, the centroid keyword does nothing.
@numberZero
Copy link
Contributor

@sfan5 I see 3 possible fixes:

  1. #define centroid in the preamble, just like #define lowp for non-GLES
  2. #if GL_ES / #ifdef GL_ES in the shader itself
  3. bump GLSL ES version (requires other changes)

@sfan5
Copy link
Member

sfan5 commented Dec 19, 2020

My preference order is 2 1 3 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants