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 "Light Only" mode of CanvasItemMaterial #75181

Merged
merged 1 commit into from
May 27, 2023

Conversation

dalexeev
Copy link
Member

Closes #44559.

@dalexeev dalexeev requested a review from a team as a code owner March 21, 2023 10:43
@@ -611,6 +611,9 @@ void main() {
}

light_blend_compute(light_base, light_color, color.rgb);
#ifdef MODE_LIGHT_ONLY
ligth_only_alpha += light_color.a;
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what should be used here, sum, average or something else. And is clamp needed here?

@dalexeev
Copy link
Member Author

dalexeev commented Mar 21, 2023

By the way, I noticed 2 more bugs:

1. Blending works differently in 3.x and 4.0 (these are 2 different projects, but it is already clear that it works differently):

EDIT: This applies to the Light Only mode, so we can consider this as the same bug.

2. In 4.0, the alpha channel of the Light2D.color property is ignored.

EDIT: In 4.0 the alpha channel is used for energy. I tried to fix it, but in 3.x alpha and energy give slightly different results (perhaps they just have different weights). Another possible solution is to add PROPERTY_HINT_COLOR_NO_ALPHA and document it.

@kleonc
Copy link
Member

kleonc commented Mar 21, 2023

Note it also needs to be fixed for GLES3: https://github.com/godotengine/godot/blob/0067578b5ba48be812b9c74421867ceba1bbc330/drivers/gles3/shaders/canvas.glsl.

@dalexeev
Copy link
Member Author

dalexeev commented Apr 1, 2023

Test projects for comparison with 3.x:

light-only-3.x.zip
light-only-4.0.zip

Note that background and canvas modulate affect the result. The difficulty is that the Light Only mode must take into account the background color.

@clayjohn
Copy link
Member

clayjohn commented Apr 11, 2023

This is on the right track, but it is still drawing the original sprite. The "Light Only" mode in 3.x only drew the contribution from the Light2D

3.x: notice how the original sprite looks very faint?
Screenshot from 2023-04-11 14-16-05

This PR: notice how the Light2D is essentially only acting as a mask
Screenshot from 2023-04-11 14-16-01

This PR is a step in the right direction. But the trouble is that the final color is ultimately mixed with the background using the blend mode of the Sprite rather than the blend mode of the light (which is kind of necessary with how "Light Only" worked.

It would be interesting to hear from people like @KoBeWi to see if this PR would be enough for their use case as I think matching the 3.x behaviour exactly may be too difficult

Edit: I guess the difference is only really noticeable with the Add blend mode:

3.x: left is with normal mode, right is with "light only" lights are using mix, sub, and add
Screenshot from 2023-04-11 14-28-27
This PR: left is with normal mode, right is with "light only" lights are using mix, sub, and add
Screenshot from 2023-04-11 14-28-17

edit: edit: Looks like you can get the 3.x behaviour with this PR by setting the Sprite's blend mode to Add and the light's blend mode to Mix

@KoBeWi
Copy link
Member

KoBeWi commented Apr 14, 2023

Well, my use-case is using lights to reveal hidden things (objects, outlines). The lights and hidden objects are using exclusive light_mask and the light texture is flat. Works perfectly with this PR.

godot.windows.editor.dev.x86_64_WlmFQy7ISB.mp4

@dalexeev
Copy link
Member Author

I marked this PR as draft for two reasons:

  1. The issue is not properly fixed, although as clayjohn pointed out, even this rough fix might be enough for basic scenarios.
  2. I planned to investigate the problem, but it's been almost a month and during this time I haven't touched it. I don't think I'll come up with any new ideas unless someone more experienced in rendering gives me a hint.

So I undraft this PR. If someone has the desire and confidence to fix it properly, then I don't mind. Let us know about it and feel free to use it as a basis.

Or we can merge this PR as is, for the reason "it's better to have a feature that doesn't work well with advanced scenarios than a feature that doesn't work at all" and fix it properly in future PRs.

@dalexeev dalexeev marked this pull request as ready for review April 26, 2023 11:41
@Janonas
Copy link

Janonas commented May 13, 2023

Would you be willing to share your compiled exe file with these fixes? Light only mode is pretty integral for my project, and i cannot continue development without it working properly. Ive tried compiling the code myself, but i must be doing something horribly wrong, as i cannot seem to get it working. It would be a huge help as the only other alternative i have is manualy porting the project back to godot 3, which would be a major headache.

@dalexeev
Copy link
Member Author

dalexeev commented May 13, 2023

Would you be willing to share your compiled exe file with these fixes?

I rebased and force-pushed the branch. When the build is over you will be able to download the artifacts.

@Janonas https://github.com/godotengine/godot/actions/runs/4965461719?pr=75181#artifacts

@Janonas
Copy link

Janonas commented May 14, 2023

Thank you, it works flawlessly for my purposes.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Let's go ahead with this. The users who are missing a "light only" mode seem to all be happy with this approach. I'm not sure that fully replicating the 3.x behaviour is possible/desirable and this approach makes a lot of sense given how 2D light rendering works in Godot 4

@YuriSizov YuriSizov merged commit f6dcd7f into godotengine:master May 27, 2023
@YuriSizov
Copy link
Contributor

Sounds good. Thanks!

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.

Light-only mode in CanvasItemMaterial makes both sprite and light disappear
7 participants