godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.04k stars 21.18k forks source link

Setting the POSITION of a vertex in a conditional statement leads to weird behavior #89869

Closed Dalayeth closed 6 months ago

Dalayeth commented 7 months ago

Tested versions

System information

Godot v4.2.1.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1660 Ti (NVIDIA; 31.0.15.5186) - AMD Ryzen 7 3800X 8-Core Processor (16 Threads)

Issue description

When setting the POSITION built-in variable in a conditional statement the vertex always seems to be affected. Regardless of the condition. Even when the condition evaluates to false the code is having an effect on the POSITION. After some testing the behavior seems unpredictable. At first even when doing if (false) made the mesh appear in the center of my screen but after some testing it sometimes disappears. Only when commenting out the code that sets the POSITION variable does it not take affect.

Steps to reproduce

  1. Create a new project with the default settings.
  2. Create a new scene with a Node3D parent.
  3. Create a GPUParticles3D node.
  4. Set the ParticleProcessMaterial to a new ParticalProcessMateria, leave the settings the default.
  5. Set Draw Passes > Pass 1 to a QuadMesh.
  6. Set the QuadMesh's Material to a new ShaderMaterial.
  7. Set the ShaderMaterial's Shader to a new shader, name it anything.
  8. In vertex function add if (false) { POSITION = vec4(-1,-1,-1,-1); }.

When the code setting the position is not commented out it still affects the particle even when the conditional statement evaluates to false.

Minimal reproduction project (MRP)

reproduce.zip

huwpascoe commented 7 months ago

The W component should be either positive 0 or 1 depending on what's being represented

point = x,y,z,0 vector = x,y,z,1

Does the issue still occur with vec4(-1,-1,-1,1)?

Dalayeth commented 7 months ago

Yes it also happens when I use vec4(-1,-1,-1,1). I don't think the w component is the issue since the if statement should always be false the code inside should have no effect. It should not be run at all. In GLSL most compilers would remove the code altogether if the statement looks like "if (false)".

jsjtxietian commented 7 months ago

I don't think the w component is the issue since the if statement should always be false the code inside should have no effect. It should not be run at all.

The problem is that it will affect the generated glsl code.

I suspect it has something to do with reading the uninited value since adding an else branch will solve the problem :

  bool m_test = false; 
  vec4 position;
  if (m_test) {
    position = vec4(0.0, 0.0, 1.0, 1.0);
  }
  gl_Position = position;

See also the preview window:

image

image

AThousandShips commented 7 months ago

This sounds like something to document, I even thought it had already been as it's a pretty standard thing in shader code, but needs documenting in the shader reference section I'd say

Accessing the parameter anywhere makes it dynamic so it is not assigned by the default way, the checks for the compiler are pretty simple, they don't check for dead paths etc., that'd be a lot more work to accomplish

In general for shaders it's important to ensure you don't have undefined paths anyway, because shaders are built to be fast, so they tend to not do safety stuff and ensure initialization etc., to keep it fast

AThousandShips commented 7 months ago

For debugging purposes I'd suggest using the preprocessor instead for this btw:

void vertex() {
#if 0
    POSITION = vec4(...);
#endif
}
clayjohn commented 6 months ago

Yes, we need to add documentation here to say that if POSITION is written to anywhere in the shader, it will always be used, so the user becomes responsible for ensuring that it always has acceptable values.