mcpiroman / UnityNativeTool

Allows to unload native plugins in Unity3d editor
MIT License
184 stars 19 forks source link

Use relative path for assemblyPaths #17

Closed rogerbarton closed 4 years ago

rogerbarton commented 4 years ago

This is not so important but similar to #16. As the Options.assemblyPaths are currently serialized as absolute paths in the scene file, when opening the project on another device the paths are incorrect and have to be set again.

Is it possible to use relative paths to avoid this, maybe use Assembly.outputPath? This only has to be for the strings stored/serialized in the scene file. Is there an easy solution to this?

rogerbarton commented 4 years ago

This is related to the asmdef's in #14, personally I only need Assembly-CSharp.dll but this is not the executing assembly in that case (hence the need for setting the assemblyPaths manually).

mcpiroman commented 4 years ago

I will need to rewrite the whole assembly resolution code anyway to support #14 and #15, so I may as well solve this. (And frankly it doesn't seem all that easy, if only I had much time for this..)

rogerbarton commented 4 years ago

I also realized that the currently the assemblyPaths won't work in a build, which is not so important but it means you can only use 'executing assembly'. So for this you would need to use a relative path or maybe even just the assembly name. I'll have a look into it in the next week or so...

rogerbarton commented 4 years ago

Using just the assembly name seems to work (in a small test of mine). This seems to be an easy solution. I haven't done the editor code yet but it would simplify it, as the user can simply enter the string of the dll name (we wouldn't really need a drop-down list).

4ff816839a69f265c78b472d0c7f07becc919f8f

Is this a good way of doing it? Not sure if the Assembly.ManifestModule.Name is the best option.

mcpiroman commented 4 years ago

The thing is there could potentially be multiple assemblies with the same names, but at different paths. Not all that likely, but possible, especially that you can include compiled (managed) dlls as your assets (and I do support mocking in them, as requested in some issue which I'm too lazy to look for). Notice that there are lot of assemblies returned from AppDomain.CurrentDomain.GetAssemblies(), coming from .net, unity, scripts and assets, so names could potentially overlap.

What I was thinking is to use either whitelist and blacklist, with relative paths to assemblies, maybe regex. With default configuration it would pick up all assemblies except these known to be built-in, like UnityEngine.dll and so on. Hint line: https://github.com/mcpiroman/UnityNativeTool/blob/6c6b17c36b33c72754d0bf2ee7e7236a68e10665/scripts/Editor/DllManipulatorEditor.cs#L205

And presumably, by default it also shouldn't pick up assemblies in assets, because they could be third-parties like Newtonsoft.Json.dll and it wouldn't be ideal to mess with them (again, by default), if only for performance reasons.

And yeeh, drop-down list doesn't seem like a must-have feature, same goes for the missing assembly checks (which is relevant in case of whitelist, I guess).

So there is some room for invention. I would like to have a look at this if I had enough time, which I hope I eventually will, but for now I'd be willing to see if, put simply, someone else would look at this too :).

rogerbarton commented 4 years ago

Of course there could be two assemblies with the same name, but there really shouldn't in the first place. I don't think this is a particularly important case to handle. Seeing as the convention is to use CompanyName.LibraryName there shouldn't be a major conflict.

I had in mind that the user specifies a list of assemblies to mock, as a string[], by default only { "Assembly-CSharp" }. The Unity/third party dll's will then not be included (unless the user specifies it).

The white/blacklist makes sense but I don't think its required for most people using this tool. We could print a warning if there are duplicate names detected.

mcpiroman commented 4 years ago

Of course there could be two assemblies with the same name, but there really shouldn't in the first place. I don't think this is a particularly important case to handle. Seeing as the convention is to use CompanyName.LibraryName there shouldn't be a major conflict.

I was thinking about pairs like debug/release, x86/x64 or editor/player, but then Unity should only load one of them. So yeah, name clash is very unlikely, although ultimately it could be slightly better to work with (relative) paths. Nothing I would block on or even do myself for a while though.

I had in mind that the user specifies a list of assemblies to mock, as a string[], by default only { "Assembly-CSharp" }. The Unity/third party dll's will then not be included (unless the user specifies it).

That's what I meant by saying whitelist :). And also it should include Assembly-CSharp-Editor, at least in case of enableInEditMode from #12.

So this could be this way (this is how it works now), but in case that someone has assembly definition files, it wouldn't pick them up, so they would need to be specified manually, by name. Secondly, recent DLL load triggers are also to be searched, also within this tool's assembly (#xx), which as of #14 wouldn't be in Assembly-CSharp, so there would need for separate search list.

Because of this it could be better to instead include everything coming from Unity's compilation pipeline (namely, assemblies with scripts) and also use that for triggers.

mcpiroman commented 4 years ago

Closed by #14