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

Shading Model - Emission Support #362

Merged
merged 5 commits into from Feb 14, 2021
Merged

Conversation

Donkeylipsh
Copy link
Contributor

Shading Model - Emission Support

Created a Shader Subgraph that applies Emission to a material using CustomEmission.hlsl, and added Emission to the Toon Shadergraph

  • Forum Thread
  • This PR completes the "Emission support" task, by providing artists with the ability to add emission effects to their objects using the base Toon shader.
  • Codecks Task
  • Without these changes, the Toon shader is unable to support emission effects
  • I created a Shader Subgraph that scales an input color, based on an emission flag, a texture mask to target emission, and an intensity value to scale the emission. If the flag and mask allow for emission, the pixel will have it's value scaled to a minimum of 1.5 (i.e. and input value of 0. (black) will have an output value of 1.5). Otherwise the color will output unchanged.
  • I then inserted the Subgraph above into the Toon Shadergraph, just before the "Unlit Master" node
  • To mask areas from emission, artists will need to set the color of the areas they don't want to emit to pure black (0., 0., 0.)

Note/Future Improvement

  • The mask texture was designed to use grayscale values, but will also allow for artists to emit a different color than the surface material if they choose. If this is a desired feature, a slider to control the mix of surface color to emission color should be added to improve this PR

vanoverc and others added 3 commits February 10, 2021 21:12
…ustomEmission.hlsl, and added Emission to the Toon Shadergraph
Shading Model - Emission Support
@CLAassistant
Copy link

CLAassistant commented Feb 12, 2021

CLA assistant check
All committers have signed the CLA.

@ciro-unity
Copy link
Contributor

Hey @Donkeylipsh! Welcome to the project, I think this is your first contribution, right? Cool.

Out of curiosity, why did you go with custom HLSL code? That looks like stuff we could do with Shadergraph. Plus, you're using an if statement which, as far as I know, is less than optimal in shaders. The funny thing is that if you were to port the same logic to Shadergraph, the resulting code wouldn't have an if because SG would use a multiplication instead, optimising it greatly.

Would you be able to convert it into a graph?

@ciro-unity ciro-unity added the art Art or music assets, or tweaks label Feb 12, 2021
@Donkeylipsh
Copy link
Contributor Author

Hi @ciro-unity! This is my first contribution, and actually my first contribution to anything that wasn't part of a class project. Which pretty much explains all the decisions I made, they were rookie mistakes. I went with the custom HLSL simply because I'm new to Unity and was more comfortable in code than I was working with all the wires and nodes. Thanks for the tip on the if statements in Shaders, that'll help optimizing all my future work.

I'll get this converted to Shadergraph and get rid of that pesky if statement tonight and resubmit my PR.

FYI, my username on the Unity forums in cdvano, so you can take a look at the screenshot I posted to see if this is working as you expected.

Thanks for all your help and advice!

@Donkeylipsh
Copy link
Contributor Author

@ciro-unity Everything has been converted to Shadergraph and works as expected.

image

@ciro-unity
Copy link
Contributor

Hahhaa what a carnival!! 😆
Thanks a lot @Donkeylipsh. Will pull it as soon as I verify the implementation.

@Donkeylipsh
Copy link
Contributor Author

Sounds good. I wasn't sure if I should include the mask textures I used for testing in the PR, so I left those out. Here's a couple masks you can use if you wanted to save some time during testing.

bard_hare_Emission
PhoenixChick_Emission
Pig_Emission
PlantCritter_Emission
RockCritter_Emission
SlimeCritter_Emission
townsfolk_F_Emission
townsfolk_M_Emission

@ciro-unity
Copy link
Contributor

I ended up simplifying the graph a bit, no need for minimum emission and the other parameter. I reduced it to the texture (which provides the colour and the difference within the object) and 1 float, for multiplication. All is added on top of the base colour.

The reason is that when somebody uses the shader, they won't understand immediately what "minimum" does, so setup now is ultra simple for everyone. Thanks anyway, merging!

@ciro-unity ciro-unity merged commit ac6b116 into UnityTechnologies:main Feb 14, 2021
@Donkeylipsh
Copy link
Contributor Author

Sounds good. Thanks @ciro-unity !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
art Art or music assets, or tweaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants