Unity-Technologies / com.unity.toonshader

Unity Toon Shader ( an experimental package )
Other
1.06k stars 168 forks source link

Directional Light Cookies Support [URP] #366

Closed Pachinko0 closed 3 weeks ago

Pachinko0 commented 1 month ago

Light Cookies are still not supported, is there any reason why?

Also, Is there any way we could add it to the shader code ourselves if its not planned to add it?

Its a really important feature for our final game look and its really sad we cant use the shader because of this barrier.

Thank you in advance.

sindharta-tanuwijaya commented 1 month ago

I am sorry to hear about the inconvenience you’ve encountered. Due to recent internal reprioritizations within Unity, we have unfortunately had to lower the priority of the ToonShader package at this time.

From exploring your issue, it seems you have two potential paths forward:

  1. Modify the shader yourself to suit your needs.
  2. Consult our professional (paid) services team for specialized assistance.

If you are interested in the second option, please let us know, and we will connect you with the team.

Thank you for your understanding and patience.

Pachinko0 commented 1 month ago

Wasnt expecting that kind of answer. I guess its time to let this shader go.

If anyone has more insightful or helpful tips feel free to share.

misakieku commented 1 month ago

You can check the urp source code and simply copy paste the code into the toon shader. https://github.com/Unity-Technologies/Graphics/blob/46939671333a1e43b771477a5a58bca85719a9c7/Packages/com.unity.render-pipelines.universal/ShaderLibrary/RealtimeLights.hlsl#L115

Pachinko0 commented 1 month ago

You can check the urp source code and simply copy paste the code into the toon shader. https://github.com/Unity-Technologies/Graphics/blob/46939671333a1e43b771477a5a58bca85719a9c7/Packages/com.unity.render-pipelines.universal/ShaderLibrary/RealtimeLights.hlsl#L115

I apreciate your clear answer.

I have copied it right after line 327 of UniversalToonBody.hlsl

if defined(_LIGHT_COOKIES)

            real3 cookieColor = SampleMainLightCookie(positionWS);
            light.color *= cookieColor;

endif

Then i have copied additional light part right after line 403

if defined(_LIGHT_COOKIES)

            real3 cookieColor = SampleAdditionalLightCookie(lightIndex, positionWS);
            light.color *= cookieColor;

endif

However it still wont render Light Cookies. Any suggestion of what i might be missing?

misakieku commented 1 month ago

I have not used the urp version since I am currently working on hdrp. But you can let the shader just output the cookieColor as the final result to determine where the problem is.

Pachinko0 commented 1 month ago

I have not used the urp version since I am currently working on hdrp. But you can let the shader just output the cookieColor as the final result to determine where the problem is.

Thank you, I forgot to define the _LIGHT_COOKIES keyword. Im not so used to hlsl but i think its almost there.

Im able to use positionWS from within AdditionalLights function declared around line 346 as UtsLight GetAdditionalPerObjectUtsLight(int perObjectLightIndex, float3 positionWS,float4 positionCS)

However how can i use it from MainLight function? It has no parameters and its defined in line 307 as: UtsLight GetUrpMainUtsLight()

misakieku commented 1 month ago

Just add a float3 positionWS parameter by your self

Pachinko0 commented 1 month ago

Just add a float3 positionWS parameter by your self

I have used positionW instead and it works beautifully!! Thank you! Im very grateful.

I dont understand why they didnt bother adding it if it was that simple.

sindharta-tanuwijaya commented 1 month ago

Thank you for the engaging discussion, and I am glad that this discussion with the community is able to resolve this issue.

As I have previously mentioned, the ToonShader team at Unity has had to lower the priority of the ToonShader package for now, including both its development and testing phases. Consequently, we currently lack the time and resources to implement new features, regardless of how straightforward they may seem.

However, the team remains passionate about ToonShader and we intend to resume its active development, which will be necessary to integrate upcoming HDRP/URP features as well. To help make this happen, we would greatly appreciate it if the community could submit requests as outlined in this article.

Thank you for your understanding.

Pachinko0 commented 3 weeks ago

Thank you for the engaging discussion, and I am glad that this discussion with the community is able to resolve this issue.

As I have previously mentioned, the ToonShader team at Unity has had to lower the priority of the ToonShader package for now, including both its development and testing phases. Consequently, we currently lack the time and resources to implement new features, regardless of how straightforward they may seem.

However, the team remains passionate about ToonShader and we intend to resume its active development, which will be necessary to integrate upcoming HDRP/URP features as well. To help make this happen, we would greatly appreciate it if the community could submit requests as outlined in this article.

Thank you for your understanding.

I genuily feel like the time you took to answer this thread was more of a waste of time than it´d took to clarify what the previous user did.

sindharta-tanuwijaya commented 3 weeks ago

I genuily feel like the time you took to answer this thread was more of a waste of time than it´d took to clarify what the previous user did.

You may be right that it could be faster for our team to resolve this issue using our free time. However, we both understand that this approach is not sustainable for the long-term development of this package. To continue its development properly, there are many other aspects we need to implement and test, especially with the release of new features in Unity.

To ensure sustainability, we need the community's involvement to provide feedback and contribute. This will help us gather user insights so Unity can allocate its resources more effectively, and your input is invaluable in this process.

By the way, I also noticed that you had closed this issue. Since we haven't addressed it yet, I would have preferred to keep it open, but I will leave the decision to you.

Pachinko0 commented 3 weeks ago

I genuily feel like the time you took to answer this thread was more of a waste of time than it´d took to clarify what the previous user did.

You may be right that it could be faster for our team to resolve this issue using our free time. However, we both understand that this approach is not sustainable for the long-term development of this package. To continue its development properly, there are many other aspects we need to implement and test, especially with the release of new features in Unity.

To ensure sustainability, we need the community's involvement to provide feedback and contribute. This will help us gather user insights so Unity can allocate its resources more effectively, and your input is invaluable in this process.

By the way, I also noticed that you had closed this issue. Since we haven't addressed it yet, I would have preferred to keep it open, but I will leave the decision to you.

I closed it because what i got from you was this is not a priority you better go off hire someone.

I understand your position and work philosophy, but it makes useless to have this collaborative platform if we are shutted off with such starting statement.

As you saw, i was able to pull it off thanks to another contributor. I would then been able then to make a request so you could look into my current solution (or not!) and maybe push the contribution (or not!).

It was just the way i got thrown out that made me not see this as the collaborative platform you praise in your last message.