Initial. AlphaTestGroupStrategy and associated test. #6317
Conversation
| @@ -0,0 +1,151 @@ | |||
| package com.badlogic.gdx.graphics.g3d.decals; | |||
There was a problem hiding this comment.
please add copyright header (like other libgdx classes) and also run formater on the whole class, see https://libgdx.badlogicgames.com/documentation/hacking/Contributing.html#formatting
There was a problem hiding this comment.
it looks like there is an issue with the formatted code (some spaces everywhere and some space padded statements)
| @@ -0,0 +1,151 @@ | |||
| package com.badlogic.gdx.graphics.g3d.decals; | |||
|
|
|||
| import com.badlogic.gdx.*; | |||
There was a problem hiding this comment.
better only import what is used instead of all packages
| import com.badlogic.gdx.utils.*; | ||
|
|
||
| /** | ||
| * <p>Single Group strategy (all Decals same) using Z-buffer to render using screen door transparency to avoid having |
There was a problem hiding this comment.
we need to explain clearly what the pros and cons (eg. doesn't support half transparency, performances impacts compared to general purpose CameraGroupStrategy), and also document it the same way as CameraGroupStrategy with a states table.
just like CameraGroupStrategy, i think it's important to explain to devs, in which situation this class is preferable over CameraGroupStrategy. Also explain a bit more the stipple/dissolve/screen door feature, especially explain that it takes both original texture alpha and decal alpha to perform the test and the stipple.
Globally, i think a clear documentation is very important for this class.
| public void beforeGroups() { | ||
|
|
||
| Gdx.gl.glEnable(GL20.GL_DEPTH_TEST); | ||
| shader.begin(); |
There was a problem hiding this comment.
shader.begin is deprecated, use bind instead
|
|
||
| @Override | ||
| public void afterGroups() { | ||
| shader.end(); |
There was a problem hiding this comment.
shader.end is deprecated, you can remove this line.
| + "varying vec4 v_color;\n" // | ||
| + "varying vec2 v_texCoords;\n" // | ||
| + "uniform sampler2D u_texture;\n" // | ||
| + "float[] bayerMatrix ={1.0f / 17.0f, 9.0f / 17.0f, 3.0f / 17.0f, 11.0f / 17.0f," // |
There was a problem hiding this comment.
f is not valid in GLSL2 shader code for literal floats.
I think it would be better to have stipple feature as an option (disabled by default). Could be a boolean in constructor.
| + "{\n" // | ||
| + "int xPos = int(gl_FragCoord.x) % 4; \n" // | ||
| + "int yPos = int(gl_FragCoord.y) % 4; \n" // | ||
| + "float stipple= bayerMatrix[ xPos * (yPos*4) ]; \n" // |
There was a problem hiding this comment.
are you sure about this? isn't it yPos * 4 + xPos instead?
There was a problem hiding this comment.
That does look incorrect, and it also won't run on GWT (can't use variable indices into arrays). It should be documented that this is GWT-incompatible.
There was a problem hiding this comment.
Good catch! indeed, variable array index requires GLSL 130 (GLES 3.0+ / WebGL 2.0). There are possible workarounds either using a for loop to find the entry or using a texture lookup.
I was balanced to include this stipple feature from the beginning mainly because it's not really a concern of group strategy. Since implementation would bring more complexity or limitation, i'm in favor to fully remove this feature from the PR.
It could be interesting to have it but i would separate it from Decals stuff (and do it in another PR), user code could plug it in this class easily by overriding this method anyway.
There was a problem hiding this comment.
From Tettinger's post the bayer stuff is no longer going to be present anyway. I have been looking at this https://bartwronski.com/2016/10/30/dithering-part-three-real-world-2d-quantization-dithering/. I think stipple should be included though, because it is a part of the group strategy idea, that within a strategy some groups are translucent, and others opaque etc but if you remove stippling then the group strategy has no support for translucency at all when it could easily be a setting in the constructor. And i think that probably people would prefer CameraGroupStrategy or AlphaTestStrategy working entirely without needing extra stuff for the second choice, because its easy to just replace one with the other to see which is preferable.
| + "int yPos = int(gl_FragCoord.y) % 4; \n" // | ||
| + "float stipple= bayerMatrix[ xPos * (yPos*4) ]; \n" // | ||
| + "vec4 tex2D = texture2D(u_texture, v_texCoords); \n" // | ||
| + "vec4 tinted = v_color*tex2D; \n" // |
There was a problem hiding this comment.
could be optimized a bit, we only need alpha multiplication for alpha test and we only need RGB multiplication for fragColor, eg. gl_FragColor = vec4(tex2D.rgb * v_color.rgb, 1.0); i'm not sure which version perform the best though, it'd need to be confirmed.
|
|
||
| /** | ||
| * <p>Single Group strategy (all Decals same) using Z-buffer to render using screen door transparency to avoid having | ||
| * to presort the decals. Materials are still sorted.</p> |
There was a problem hiding this comment.
presort decals is not an issue, the reason of this class is to avoid overlap artifacts in some situation.
|
|
||
| import java.util.*; | ||
|
|
||
| public class DecalAlphaTest extends GdxTest { |
There was a problem hiding this comment.
This test is a bit complicated, a load test is not really what we need. Maybe a better approach would be to switch from/to different strategy at runtime and see how much FPS we can get (vsync can be disabled manually by testers when needed). Also adding/removing decals manually (via keyboard keys) would be better.
Before performance measurement, what we really need is to test that the strategy works (eg. the stipple effect, etc)
| @@ -147,6 +147,7 @@ | |||
|
|
|||
| <!-- graphics/g3d/decals --> | |||
| <include name="graphics/g3d/decals/CameraGroupStrategy.java"/> | |||
| <include name="graphics/g3d/decals/AlphaTestGroupStrategy.java"/> | |||
There was a problem hiding this comment.
please use alphabetical order (A before C)
| + " gl_Position = u_projectionViewMatrix * " + ShaderProgram.POSITION_ATTRIBUTE + ";" + "\n" // | ||
| + "}\n"; | ||
|
|
||
| String dfragmentShader = "#ifdef GL_ES\n" // |
| * @param bayerFilter Specifies the size of the dithering filter used to discard pixels | ||
| * of translucent textures. | ||
| */ | ||
| public AlphaTestGroupStrategy (final Camera camera, final BayerFilter bayerFilter) { |
There was a problem hiding this comment.
better use an integer (keep it simple)
| * @param camera | ||
| */ | ||
| public AlphaTestGroupStrategy (final Camera camera) { | ||
| this(camera, BayerFilter.MODERATE); |
There was a problem hiding this comment.
should be disabled by default
| + "#define MAX_LEVEL 4\n" // | ||
| + "float GetBayer() {\n" // | ||
| + " float finalBayer = 0.0;\n" // | ||
| + " for(int i = 1 - "+bayerFilter.size +"; i<= 0; i++) {\n" // |
There was a problem hiding this comment.
better have an optimized version when stipple is disabled
| + " for(int i = 1 - "+bayerFilter.size +"; i<= 0; i++) {\n" // | ||
| + " float bayerSize = exp2(float(i)); // negative exponent makes an inverse\n" // | ||
| + " vec2 c = mod(floor(gl_FragCoord.xy * bayerSize), 2.0);\n" // | ||
| + " float bayer = 2. * c.x - 4. * c.x * c.y + 3. * c.y; // borrows factors of the summed geometric series soon to" |
There was a problem hiding this comment.
comments in generated shader code is not necessary, you could move it to java comments instead
| + "uniform sampler2D u_texture;\n" // | ||
| + "\n" // | ||
| + "#define MAX_LEVEL 4\n" // | ||
| + "float GetBayer() {\n" // |
There was a problem hiding this comment.
why calculating for every fragment when we could use a lookup table (like in your original PR)?
There was a problem hiding this comment.
this one works on webgl and the lookup one can't unfortunately as variable access to arrays not allowed. and for four iterations wtih this you get the equivalent of 16x16 dither block (a single pass looks ok imo)
| + "\n" // | ||
| + "void main()\n" // | ||
| + "{\n" // | ||
| + " v_color = " + ShaderProgram.COLOR_ATTRIBUTE + ";\n" // |
There was a problem hiding this comment.
missing v_color.a = v_color.a * (255.0/254.0);, see CameraGroupStrategy
…ifacts better. add GWT test
…/libgdx into AlphaTestGroupStrategy
AlphaTestGroupStrategy. DecalAlphaTest shows intersecting overlapping translucent decals without artifacts with stipple/screen door transparency. The test is based on the CameraGroupStrategy one switched to perspective camera.