amplitude / unity-plugin

Official Amplitude Unity Plugin
https://developers.amplitude.com/docs/unity
MIT License
44 stars 34 forks source link

Asmdef file in root of editor folder #77

Closed pgorrow closed 3 years ago

pgorrow commented 3 years ago

From: https://docs.unity3d.com/2018.3/Documentation/Manual/ScriptCompilationAssemblyDefinitionFiles.html

Add an assembly definition file to a folder in a Unity Project to compile all the scripts in the folder into an assembly. Set the name of the assembly in the Inspector.

Adding AmplitudeSDKEditor.asmdef to the root of the editor folder not only creates an assembly of all files in that foler, it changes the dependency path of files and subfolders within the that folder. The addition of this file breaks any code that lives in the Editor folder but has external dependencies (other plugins, user editor code, etc). If you want to use an asmdef file, it might be better to install your files into Assets/Editor/Amplitude (and put the asmdef in that directory), so that it creates an assembly and a new dependency path for only your files.

dantetam commented 3 years ago

Hello @pgorrow ,

Thanks for the suggestion, this is a good thought. Just to clarify, why would this conflict with other Editor files in other asmdef declarations, like the other plugins example you mentioned? We actually did run into this issue when the asmdef file was named Assets/Editor/Editor.asmdef.

Dante

pgorrow commented 3 years ago

The default behavior of the Editor folder is to have a dependency path to the rest of the assets folder. This allows you to create editor panels that interact with application scripts. The asmdef you added changes the default behavior of the editor folder and all folders beneath editor. As you suggested, other plugins that use an asmdef are unaffected. But most plugins that install in editor don't use an asmdef and many of them depend on the default dependency path.

In general, I'd advise against changing the behavior of default directories as it is bound to cause some people pain.

While I understand, you are using it to enforce modularity. An asmdef is mostly useful if you have a large body of code that takes significant time to compile and you want to limit how frequently it compiles.

pgorrow commented 3 years ago

Since Editor is a special folder name (https://docs.unity3d.com/Manual/SpecialFolders.html) and you can have multiple Editor folders in a project. Another possible solution would be to create an Editor folder under Assets/Amplitude with your Editor resources. This would put put them in the same module created by AmplitudeSdkCore.asmdef and move more of your code into a single asmdef module.

Zamaroht commented 3 years ago

Hello. This is a problem when importing the plugin manually via the .unitypackage file. Right now the unitypackage is importing the package.json file (which is irrelevant), and it's adding these two files:

Assets/Plugins/AmplitudeSDKPlugins.asmdef
Assets/Editor/AmplitudeSDKEditor.asmdef

The problem is that Assets/Editor and Assets/Plugins are commonly used folders for project code and other SDKs, and these asmdefs are blocking all the code inside these folders from seeing the rest of the project code, leading to compilation problems when importing the package in an existing project.

This shouldn't be a problem via Package Manager, but since .unitypackage seems to be supported this should be fixed.

dantetam commented 3 years ago

Hello @Zamaroht ,

Those are very interesting results, that makes sense it would only conflict with "default editor" plugins in the .unitypackage setup (and not Unity Package Manager). Let me discuss with my team about the best way to move forward. There's quite a bit of architecture that relies on the current file setup. Moving the files to Assets/Editor/Amplitude and ensuring the SDK still works will most likely be the best way forward. We're targeting 1-2 weeks for this change.

Thank you again for the thorough investigation.

Dante

Zamaroht commented 3 years ago

Just in case anyone else is experiencing compilation errors because of this after importing the .unitypackage, deleting both .asmdef files should fix it in the meantime.

dantetam commented 3 years ago

Hello @Zamaroht ,

Sorry for the delay. I'm going to experiment with not including the asmdef files in the unitypackage. Our old unitypackage file did not require them before. The asmdef files were made for Unity Package Manager. This might resolve all the issues