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 emission calculation for spatial shader #10441

Closed

Conversation

maxim-sheronov
Copy link
Contributor

I think, that current emission calculation is incorrect, or at least not convenient.
Current logic sums color and value from texture. That means, that you can't use texture as mask to make some parts of mesh emitting. Or you can use black color on texture to not emit, but color become useless. And you can't use it as tint to colorize emission.

I suggest to use texture as mask by multiplying it with color instead of summation.

Also I changed hint color for case, when you don't need a texture.

@Zireael07
Copy link
Contributor

and here I was wondering why my emission mask does nothing in 3.0, thanks man!

@akien-mga akien-mga added this to the 3.0 milestone Aug 19, 2017
@fracteed
Copy link

Excellent! I have been meaning to create an issue for this. Look forward to trying it out :)

@reduz
Copy link
Member

reduz commented Aug 20, 2017

I thought it might be more useful the current way, but if you prefer this way I'm fine too. PR looks perfect.

@reduz
Copy link
Member

reduz commented Aug 21, 2017

I was thinking about it, and I'm not sure if this proposal is better. The rationale is the following:

  • Emission and Emission texture in general are not used together, this is rare
  • If using a multiplication, you will need to set emission to white besides setting a texture for it to work, I don't think this is very intuitive.
  • The only benefit I see this could have is controlling the intensity of the emission, but you can already do this with the energy param

So I think after thinking about it for a while, that this PR is a bad idea, it just makes emission more restricted.

@ghost
Copy link

ghost commented Aug 21, 2017

Isn't the current behavior similar to how emission map works in Unity?

@maxim-sheronov
Copy link
Contributor Author

maxim-sheronov commented Aug 22, 2017

@reduz, I think the main benefit of multiplying is ability to change Color of emission using texture as mask.
And without multiplication you just can not colorize your emission by script. I think @Zireael07 and @fracteed was talking about that case.

And other engines works this way.
@tagcup, Unity works like in this PR, they uses multiplication.

@Zireael07
Copy link
Contributor

I was mostly talking about being able to use black/white masks to control which parts of the mesh do emission (eg. only a lamp or a decorative border)

@maxim-sheronov
Copy link
Contributor Author

@Zireael07 yes, but to have control on light color of that emission you need multiplication of emission texture and emission color

@ghost
Copy link

ghost commented Aug 24, 2017

Then this would also mean that's how it works with Substance, right?

@reduz
Copy link
Member

reduz commented Aug 28, 2017

I'm still not convinced that it's more intuitive, I would probably just add an option to select the behavior..

@ghost
Copy link

ghost commented Aug 29, 2017

I'm not exactly sure how this works with Unity or Substance (is there an official doc about it?), but based on this discussion, the current behavior in Godot apparently matches Khronos shader in their gltf exporter repo.

@ghost
Copy link

ghost commented Aug 29, 2017

The current Godot implementation is also the one that physically makes more sense: in the absence of emission you should only get albedo (which is the reflected environmental light), and when you turn on an additional light source (which is the emission) it should just add to albedo.(electromagnetic waves add without interacting, neglecting quantum mechanical effects for bosons. also in reality, when you turn on the light source it will heat the object up changing transport properties a little such that the albedo will be slightly different but shouldn't be much.)

@fracteed
Copy link

@reduz I would imagine that this is directly related to #10534

@maxim-sheronov
Copy link
Contributor Author

@reduz, I really don't know in what case it is convinient to have sum of emission color and texture. Maybe you can give an example of such use case? If it can be helpful in some situation, maybe to have an option is better idea. Because with current implementation it is impossible to do coloring of emission with mask.

@tagcup About Unity you can see here
You can see on screenshots, that when they use texture, they set color to white, because of multiplying.

@ghost
Copy link

ghost commented Aug 29, 2017

I don't know whick picture you're referring to exactly but this one (from that page) is mostly black:

https://docs.unity3d.com/uploads/Main/StandardShaderEmissiveMaterialInspector.png

@Zireael07
Copy link
Contributor

@tagcup: That's because it's partially a mask. Now if I were to apply this same picture as emission texture in Godot, the whole mesh would be emissive right now in 3.0 (and properly masked in 2.1.3).

@reduz
Copy link
Member

reduz commented Aug 29, 2017

Given there are good reasons for both use cases, I will do the following

  • Given how Godot exports the parameters, the current usage is more intuitive for creating materials, it allows setting a texture and making sure it works on the first try
  • Also given there is a valid use case in changing the emission mask color, I will add this as an extra option in the material.

@ghost
Copy link

ghost commented Aug 29, 2017

@Zireael07 I must've misunderstood what Godot is currently doing then. So according to what you're saying, Godot is currently adding albedo as emissive color on top of the given emissive texture

@maxim-sheronov
Copy link
Contributor Author

maxim-sheronov commented Aug 29, 2017

@tagcup, as @Zireael07 sad, result is almost black. But on the right side you can see almost black texture with small green and orange squares, and to the left you will see white color

@tagcup, in current implementation Godot use color from texture plus emission color (and maybe plus Albedo). So when you are using emission texture, you can left emission color black. But in Unity you must set it to white, or other color to do coloring.

@reduz, good to know, that there will be an option to select behavior. Thanks. Also I could do this option in this PR. Or do you want to do it yourself?

@maxim-sheronov
Copy link
Contributor Author

@reduz , I have just added flag to select to use multiplication or not. Could you see is it good enough option to accept?

@maxim-sheronov maxim-sheronov force-pushed the fix_spatial_emission branch 3 times, most recently from e27aefd to 5290acd Compare September 6, 2017 16:44
@hpvb
Copy link
Member

hpvb commented Sep 12, 2017

Can you please squash the clang-format commit? It doesn't make much sense for the public git history. Other than that, @reduz is the multiplication flag that was implemented after your comment a good fix, should this be merged in this form?

@Zireael07
Copy link
Contributor

What are we doing with this? Still unable to mask my emissions as of alpha 2...

@HummusSamurai
Copy link
Contributor

I think this will be merged eventually since it adds a flag for users to choose whether they want additive or multiplicative mode.

@reduz reduz closed this in 7010ee3 Nov 15, 2017
@maxim-sheronov maxim-sheronov deleted the fix_spatial_emission branch December 8, 2017 06:13
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.

7 participants