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

Added support for PluginAPI #564 #597

Merged
merged 39 commits into from
Oct 30, 2023
Merged

Added support for PluginAPI #564 #597

merged 39 commits into from
Oct 30, 2023

Conversation

igor84
Copy link
Collaborator

@igor84 igor84 commented Oct 24, 2023

No description provided.

@JoC0de
Copy link
Collaborator

JoC0de commented Oct 24, 2023

I don't like binaries inside GIT, they can get out-of-sync.
Shouldn't it be possible without .dll inside GIT?

  • Place the PluginAPI source code inside a subdirectory of src/NuGetForUnity/Editor/ so Unity knows it and it is included inside the UPM package
  • The NuGetForUnity.Cli and NuGetForUnity.CreateDll shouldn't have a issue with it
  • Create a NuGetForUnity.PluginAPI.csproj that we can build separately and provide as a download / publish as a NuGet package

So others can create the Plugin either by:

  • installing the NuGet package
  • copying the DLL (not recommended)
  • directly inside Unity

See how Unity allows "expendability" e.g. with the code:
https://github.com/needle-mirror/com.unity.ide.visualstudio/blob/b28f5a3cbf26211491a4f3db4561300da94383a9/Editor/ProjectGeneration/ProjectGeneration.cs#L419-L434
They call the code of my 😃 Unity Plugin:
https://github.com/JoC0de/UnityVisualStudioSolutionGenerator/blob/2d2f01faa73161ab8b59a18b072a8f709791769b/src/UnityVisualStudioSolutionGenerator/Assets/Editor/VisualStudioAssetPostprocessor.cs#L83

@igor84
Copy link
Collaborator Author

igor84 commented Oct 24, 2023

Bellow is my reasoning for having to include dll. Correct me if I understood something wrong:

We encourage installing NugetForUnity as a UPM package. Installing that way would install the contents of Editor folder as source into user's project. That means PluginAPI would be installed as source as well. But I created some plugin that is compiled into a dll and inside that dll it is recorded that it depends on NugetForUnity.PluginAPI.dll but now there is no such dll in the project.

Maybe this would work if we can make Unity compile asmdef for PluginAPI before it tries to load the plugin dll but I am not sure that is possible.

@JoC0de
Copy link
Collaborator

JoC0de commented Oct 24, 2023

So the goal is to be able to implement the plugin in a external .csproj and not being limited to only implement it inside the same or a different unity project?

@igor84
Copy link
Collaborator Author

igor84 commented Oct 25, 2023

The plugin has to be a dll (compiled from external csproj) so that it can also be loaded from CLI version.

Comment on lines 79 to 82
run: git diff --exit-code ./src/NuGetForUnity/Editor/PluginAPI/NuGetForUnity.PluginAPI.dll

- name: Ensure NuGetForUnity.PluginAPI.xml in GIT is up-to-date (if it fails pleas build the NugetForUnity.PluginAPI.csproj and upload the changed files to git)
run: git diff --exit-code ./src/NuGetForUnity/Editor/PluginAPI/NuGetForUnity.PluginAPI.xml
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the intent here? Will it check that each commit has changes in these files? They should only be updated if something in PluginAPI project was changed which should be pretty rare.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No they should only check if NuGetForUnity.PluginAPI.dll is updated if something it the code of the PluginAPI project was changed. It currenlty has the issue that the .dll contains the version number so it always failes.

@igor84
Copy link
Collaborator Author

igor84 commented Oct 29, 2023

@JoC0de thank you very much for helping polish this PR 💪

Copy link
Collaborator

@JoC0de JoC0de left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am done with my changes 😄
The implementation looks good 👍
I already updated the version number in the source code to 4.0.2 so we can release it after merging.

@igor84 igor84 merged commit bf7df71 into master Oct 30, 2023
12 of 14 checks passed
@igor84 igor84 deleted the feature/plugin-support branch October 30, 2023 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants