-
Notifications
You must be signed in to change notification settings - Fork 199
[DONTMERGE] Unity chan #592
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
base: master
Are you sure you want to change the base?
Conversation
tigran-sargsyan-w
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Review – [DONTMERGE] Unity chan (#592)
Summary
Thanks for putting this together — this PR introduces a full UnityChan URP sample that nicely showcases Toon Shader with animation, wind and UI tooling.
What looks good
- Clear folder structure under
Samples~/URP/UnityChanwith Common/SD split. - Reuse of existing tooling (SpringManager, animation previewers, etc.) instead of ad‑hoc code.
- Integration with the new Input System to drive the sample.
Suggestions / questions
-
PR size & scope – This is a very large change (+11k / −4.8k, 238 files) that mixes scripts, binary assets, project settings and test project updates. If possible, consider splitting into:
- one PR for the sample content itself (scenes, prefabs, scripts),
- another for test project /
ProjectSettingschanges.
-
Licensing / credits – Since UnityChan assets come with their own license/attribution requirements, it would be good to:
- add a short
README.mdin the sample folder explaining the license, - include links to the original UnityChan terms and any required credits.
- add a short
-
Dependencies – The sample appears to rely on the
Input Systempackage. It might help to:- explicitly document this in a sample README,
- ensure the sample asmdef (if any) marks
com.unity.inputsystemas a dependency so users don’t hit missing‑package errors.
-
Namespacing – Some of the newly added scripts likely live in a fairly generic namespace or the global scope. To avoid clashes in user projects, consider putting them under a more specific namespace (for example
Unity.ToonShader.Samples.UnityChan). -
Docs / onboarding – A short “Getting started” section (open
ToonShader.unity, how to toggle preview UI, input bindings, etc.) would make it much easier for users to explore the sample.
Overall, this looks like a very cool demo of Toon Shader in a real character scenario — with a bit of trimming and documentation it should be much easier to review and ship.
|
Hi, Thank you very much for your review—I really appreciate it. You’re right that this PR is quite large and it may still grow. I like your suggestion to split it into smaller chunks, and I’ll follow that whenever possible.
You’re correct; we should have included them earlier. Regarding the sample: it doesn’t strictly depend on the input system package. You can remove that package and the sample should still work.
The sample was taken directly from the Unity-chan website Given that, your feedback will be valuable for us to consider when we update the version hosted on that site. |
3106a90 to
b21b7ba
Compare
[skip ci]
[skip ci]
[skip ci]
[skip ci]
b21b7ba to
01033c5
Compare
No description provided.