iBicha / ImageEffectGraph

Image effects for post processing stack created with shader graph for Unity
MIT License
168 stars 18 forks source link

Support install from package manager #9

Open edwin0cheng opened 5 years ago

edwin0cheng commented 5 years ago

In 2018.3.0b7 added support for git package dependencies from package manager : https://forum.unity.com/threads/git-support-on-package-manager.573673/

Here is the guide to make a custom package: https://gist.github.com/LotteMakesStuff/6e02e0ea303030517a071a1c81eb016e

Do you think this is a good idea to support it ? If so, i could try to make a PR.

iBicha commented 5 years ago

Hi Edwin, Thanks for opening the issue! While this is a great idea (and I was processing it for a while) but there are a few details (to consider before moving forward) that kept me from implementing this:

  1. Dependencies: while ShaderGraph is a required dependency for example, HDRP and LWRP should be optional requirements (so that shaders are used for these render pipelines only if there are installed, because in a real project you'd either have one, or no SRP, but probably never both). I don't think this is supported with UPM, which leads me to think probably the best option is to use CCU

  2. Versioning might be tricky as well, because I kept updating this repo according to updates happening to SRP, PostProcessing Stack, ShaderGraph, etc... And each time the updates break ImageEffectGraph, and I had to make modifications again. Having versioning done with optional dependencies might be tricky as well.

  3. Package validation: Unity uses a package to validation other packages it publishes, and my plan was to respect these rules as much as possible (file structure, versioning, change log, etc). There are two problems here: One, it seems that these rules are not final and keep changing (which is fine, we can adapt) and Two, it's not clear that some of these rules apply to all packages, or only packages made by Unity (for example, package names must start with "com.unity", and maybe this is to strictly identify that this is a package to be used by Unity, or that this is made by Unity)

  4. Releases: Another thing to figure out is a clean way to distribute versions through git. I do want to keep the master containing a Unity project, so people can simply download and test immediately, while other branch(es) (and/or tags?) can be used to publish the actual package. (and example would be Klak, with a master branch, and a upm branch). So a robust system to handle all this would be preferred, or else it would not be productive to people trying to fix broken dependencies.

  5. Git support is actually not final yet, and might change anytime, making all this planning (potentially) go to waste.

That guide is useful to get started, but there are few points that needs to be clarified (by Unity people) so we can build on top of that.

If not this is what I think about the subject, but if you feel confident about making this work, that would be really cool and helpful!

edwin0cheng commented 5 years ago

Hi,

Thanks for your detail reply. All your points are valid and i agreed that it is kind of waste of time at this moment. Do you recommend any installation method which others (like me) to use this project as a library ?

How about git-submodule to package folder , Unity asset package or others ?

iBicha commented 5 years ago

To be honest, UPM packages are the ideal, at least when some of these concerns are addressed. If I was to manage dependencies in a future/recent project, I would do them temporarily manually until I'm ready to migrate to using UPM. This is probably a matter of preference, but to me unity asset packages are just slow (since they are zip files), and they feel too much manual work for no real benefit. Finally, while git submodules is a viable option, I did not experiment with them to handle Unity packages, so I don't know if they would cause any sort of problems

edwin0cheng commented 5 years ago

I finally manage to convert it to using UPM :

https://github.com/edwin0cheng/ImageEffectGraph/tree/support_upm

Which solve parts of your concerns:

  1. Right now only ShaderGraph and PostProcessing are required packages. We seem to be not depends on SRP.
  2. The version of depending package are set in the package.json which we can update later as we want to support later version.
  3. I don't know how to handle the validation part. As It is still not open to other use.
  4. I basically just move the ImageShaderGraph inside the Package/com.github.ibicha.image-effect-graph and fixed minor path related bugs. And now we can use :
    git subtree split --branch upm --prefix Package/com.github.ibicha.image-effect-graph/

    to update the upm branch and release it cleanly.

And here is a demo project which you can play around and test it : https://github.com/edwin0cheng/TestISG

iBicha commented 5 years ago

Awesome, thank you for giving it a try, I'll try to review it as soon as I can (probably over the weekend)

iBicha commented 5 years ago

Hi Edwin, My apologies for being AFK, I finally had a bit of time to look at your package approach, and it's great work!

If you would like to contribute your changes through a pull request, I have few remarks:

Thank you very much!