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

Add light affect debug draw mode #27914

Closed
wants to merge 3 commits into from

Conversation

kaadmy
Copy link
Contributor

@kaadmy kaadmy commented Apr 11, 2019

Adds a new debug draw mode for viewports named "Light Affect", this is similar to overdraw but changes the color depending on how many lights currently affect the object being colored.

Currently this has only been implemented in the GLES3 backend (no other debug draw modes seem to work with GLES2 anyway, so I didn't consider this a big deal)

If the light count is increased, this will also need to change to accommodate a higher range.

  • 0 lights: gray
  • 1-4 lights: blue
  • 5-6 lights: purple
  • 7 lights: yellow
  • 8+ lights: red

Leftmost sphere has zero lights, rightmost sphere has 8 lights
scrot_region_20190410_195339

3D viewport settings
foo

ZIP file for above scene
light_affect_debug.zip

@kaadmy
Copy link
Contributor Author

kaadmy commented Apr 11, 2019

So apparently there's no way to use unshaded mode in a shader with the light function, so the geometry is still lit from GIProbes, lightmaps, etc...

@kaadmy
Copy link
Contributor Author

kaadmy commented Apr 11, 2019

Screenshot of this debug draw mode with the TPS demo
scrot_region_20190410_214227

For that screenshot I still needed to change environment settings to get the colors into a reasonable range (disabled color correction, reduced exposure to ~0.5, disabled DoF and glow)


default_light_affect_debug_shader = storage->shader_create();
storage->shader_set_code(default_light_affect_debug_shader, "shader_type spatial;\nrender_mode ambient_light_disabled,blend_add;\n void light() { SPECULAR_LIGHT.r+=1.0; int lc=int((SPECULAR_LIGHT.r)+0.1); if(lc<=4) { DIFFUSE_LIGHT=vec3(0.1, 0.3, 0.5); } else if(lc<=6) { DIFFUSE_LIGHT=vec3(0.3, 0.1, 0.7); } else if(lc<=7) { DIFFUSE_LIGHT=vec3(0.8, 0.6, 0.1); } else if(lc>=8) { DIFFUSE_LIGHT=vec3(1.0, 0.1, 0.1); } }\n void fragment() { SPECULAR=0.0; ALBEDO=vec3(1.0); EMISSION=vec3(0.1,0.1,0.1); ALPHA=0.4; }");
default_light_affect_debug_material=storage->material_create();
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, missed that somehow, it wasn't intentional.

@kaadmy
Copy link
Contributor Author

kaadmy commented Apr 11, 2019

Should this PR also disable tonemapping/environment while the debug mode is enabled? It might be simpler since showing lighting isn't the goal here (could also make this more shaded, since it might get hard to navigate scenes with this mode enabled.)

Edit: Wording

@reduz
Copy link
Member

reduz commented Apr 20, 2019

This is pretty interesting, but not very doable on GLES2, not sure if it will be doable on Vulkan either (could try).

@kaadmy
Copy link
Contributor Author

kaadmy commented Apr 21, 2019

Just tested this, the light function doesn't actually override lighting in GLES2, since specular etc seems to be modified by the shader regardless of having a custom light function. This debug mode actually just uses a regular shader (same syntax as Godot's shading language), and can be used on any material when using the GLES3 backend. As long as Vulkan behaves similar to GLES3, it should work fine.

This running on 3.1 stable with GLES3:
scrot_region_20190420_191705

@akien-mga
Copy link
Member

Commits should be squashed together.

@clayjohn Any idea on the feasibility of this feature for GLES2?

@clayjohn
Copy link
Member

@akien-mga I think it is possible. But most of the work would have to be in the cpp code. In GLES2 each light draws the object an additional time, so changing color based on number of lights doesn't quite work as each iteration of the light shader cannot rely on previous iterations as it does in this PR. However, you may be able to set a custom shader like this, say an unshaded shader, then set the albedo color based on the number of lights by providing a uniform.

That being said, none of the other debug draw modes have been ported over yet. So including this in GLES3 only isn't really a big issue.

@kaadmy
Copy link
Contributor Author

kaadmy commented Aug 29, 2019

Since there seems to be some activity for this PR I'll weigh in a bit and say that some like what @clayjohn mentioned would be a lit cleaner, ie. just pass in the light count as a uniform. I can't think of any other uses for the uniform but it'd be a lot simpler to port to other renderers.

@clayjohn
Copy link
Member

IMO we should merge it as is and then deal with GLES2 at the same time as the other debug draw modes.

@kaadmy
Copy link
Contributor Author

kaadmy commented Aug 29, 2019

If this gets merged then maybe there should be a bit more work to get this to work well without environment changes by the user, for example right now this may require custom post processing/environment settings (see the notes on the above screenshot for the TPS demo) to work correctly.

@clayjohn
Copy link
Member

I see, I missed that comment. Yea, more work will need to be done to disable those effects when using this draw mode. It should be possible to do it within rasterizer_scene_gles3.cpp though.

Let me know if you need some help finding where you need to disable stuff/change stuff and I can look into it a bit for you.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Nov 7, 2019
@aaronfranke
Copy link
Member

@kaadmy Is this still desired? If so, it needs to be rebased on the latest master branch, and redone to support Vulkan.

If not, abandoned pull requests will be closed in the future as announced here.

@aaronfranke
Copy link
Member

This PR has not received any new commits for over a year, closing. If this PR is still desired, it would need to be redone to support Vulkan as per the above discussion.

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.

6 participants