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

Asmdef file in root of editor folder #77

Closed
pgorrow opened this issue Jun 29, 2021 · 7 comments · Fixed by #83
Closed

Asmdef file in root of editor folder #77

pgorrow opened this issue Jun 29, 2021 · 7 comments · Fixed by #83

Comments

@pgorrow
Copy link

pgorrow commented Jun 29, 2021

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
Copy link
Contributor

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
Copy link
Author

pgorrow commented Jul 14, 2021

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
Copy link
Author

pgorrow commented Jul 15, 2021

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
Copy link

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
Copy link
Contributor

dantetam commented Jul 22, 2021

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
Copy link

Zamaroht commented Jul 22, 2021

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
Copy link
Contributor

dantetam commented Aug 9, 2021

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

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 a pull request may close this issue.

3 participants