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

Add upm support #14

Merged
merged 21 commits into from May 1, 2020
Merged

Add upm support #14

merged 21 commits into from May 1, 2020

Conversation

rogerbarton
Copy link
Contributor

@rogerbarton rogerbarton commented Mar 21, 2020

Users can then nicely add the repo as a submodule in the Packages folder and so get updates.

  • Add main author to packages.json @mcpiroman
  • Update readme
  • Adapt "Only Executing Assembly" to reference "Assembly-CSharp.dll", because of the .asmdef
  • Should .meta files be included (for consistent guids)? Yes

If users do not want to use upm, e.g. if they are on an older version, it should still work.

(The "revision": "6c6b..." is currently the lastest commit on the master branch)

Allow users to add this to the Packages folder for it to be used as an embedded package
Users can then nicely add the repo as a submodule
rogerbarton added a commit to rogerbarton/UnityNativeTool that referenced this pull request Mar 21, 2020
This would not work with the `upm-support` branch mcpiroman#14 as the attributes are not searched for in the attribute that these scripts are in by default.
@rogerbarton rogerbarton mentioned this pull request Mar 21, 2020
rogerbarton added a commit to rogerbarton/UnityNativeTool that referenced this pull request Mar 21, 2020
This would not work with the `upm-support` branch mcpiroman#14 as the attributes are not searched for in the attribute that these scripts are in by default.
rogerbarton added a commit to rogerbarton/UnityNativeTool that referenced this pull request Mar 21, 2020
This would not work with the `upm-support` branch mcpiroman#14 as the attributes are not searched for in the attribute that these scripts are in by default.
@mcpiroman
Copy link
Owner

Well, admittedly, I've never heard of upm, but it looks really cool. (I guess it works similarly to other package managers I know ;)). So yes, thanks for bringing that in. The only thing I'm worried about are the .asmdef files. Are they necessary? Because as you pointed out, they may interfere with assembly resolution (and for this little code there is here, this seems like little overkill?). I'm nevertheless not familiar with that solution, so I'll rely on your words.

@rogerbarton
Copy link
Contributor Author

Yes upm is based on npm (it's been around since 2018.1 I believe). From what I know the you need an asmdef per package and a separate one for any editor code, which we have here. It is overkill to have them but it doesn't work without them.

EditorGUI for the target assemblies is now just a string array. Removed code to find _allKnownAssemblies.
assemblyPaths renamed to assemblyNames is now a list.
By default only Assembly-CSharp and the Editor variant is targeted. The assembly containing this tool is always included so we find the attributes.
Simplified finding all assemblies, moved to RefreshAllKnownAssemblies()
Unity Assemblies are ignored by default.
Option to add all known assemblies.
Increased refresh time to 5 seconds from 1.
@rogerbarton
Copy link
Contributor Author

rogerbarton commented Apr 7, 2020

a5fd455 also links to #17 (comment) and implements the whitelisting of assemblies, by their name (without extension). By default Assembly-CSharp and Assembly-CSharp-Editor are included. Our two asmdefs are always included, and so searched for the attributes. So #17 should be resolved by this.

(042c4aa and 06abb19) I've now adapted the GUI for selecting the assemblies, it should be quite similar to before with a dropdown. I've slightly modified how _allKnownAssemblies is calculated, so Unity assemblies are ignored (theres a IGNORED_ASSEMBLY_PREFIXES string[] for exact control). The Find dropdown allows you to select from this list.
image

@rogerbarton
Copy link
Contributor Author

Added some headers and reordered. Any suggestions?
image

rogerbarton and others added 2 commits April 8, 2020 18:39
…e UnityEditorInternal dependency

Use original code to find all assemblies, use `Path.GetFileNameWithoutExtension` instead of `PathUtils.NormallizeUnityAssemblyPath`.
@mcpiroman
Copy link
Owner

Added some headers and reordered. Any suggestions?

Yea, I very like that,, maybe because I have thought about sth like this either :).

Generally, please let me know when I can review this. Btw Github allows you to declare a PR as preview, idk if they have already enabled marking existing ones as preview, which was requested a long time ago.

@rogerbarton
Copy link
Contributor Author

Ah I didn't know about the marking as preview, I'll do it in the future. This PR and #15 aren't ready for review yet. I'll tell you when they are...

Some project may not have any editor scripts so will always get an error as Assembly-CSharp-Editor does not exist.
@rogerbarton
Copy link
Contributor Author

Once #15 is merged, this PR is also ready for review.

mcpiroman pushed a commit that referenced this pull request May 1, 2020
* Repaint editor GUIs on shortcut

* Editor variable changes saved properly

Before changing a variable would not be detected and so not saved/serialized in some cases.

* Use callback attributes to repaint editors

This would not work with the `upm-support` branch #14 as the attributes are not searched for in the attribute that these scripts are in by default.

* Allow multiple un/loadTrigger attributes, use action to trigger repaints

* Remove SceneManagement, it's not required

The scene is automatically set as dirty when the gui target is

* Fixes to gui with enableInEdit mode

Admittedly this should have been in #12. However, here are the fixes anyways.

GUI buttons to un/load show in edit mode if enableInEditMode is true. Disabling and re-enabling the DllManipulatorScript works now.

* Properly reset custom triggers (fix duplicates)

Previously triggers would not be cleared properly and so be duplicated. (Static variables seem to persist between entering/exiting playmode)

* Custom triggers optional execute on main thread

Allow custom triggers to be executed on the main thread in a queue. We need this internally to repaint the editor GUI as it uses the Unity API.

* Update README.md

* Mark scene dirty only if options changed

* Bug fix, resetting when not initialized

Initialize() is not always called so we should not always reset.

* small fix

* Review fixes

Also RegisterTriggerMethod receives attribute instead of bool, 
Fix naming in DllManipulatorOptions

* Add comments
package.json Outdated
],
"category": "Unity",
"author": {
"name": "John Doe",
Copy link
Owner

Choose a reason for hiding this comment

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

If I recall correctly, that's not my name :)

Copy link
Owner

Choose a reason for hiding this comment

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

Nor an email

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where can I find them? they're not in your public profile.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we can live without that unless that's somehow mandatory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove the author field if you want it to stay anonymous? (fyi John Doe is just a placeholder name)

return PInvokes_Linux.dlopen(filepath, (int)Options.posixDlopenFlags);
#elif UNITY_STANDALONE_OSX
return PInvokes_Osx.dlopen(filepath, (int)Options.posixDlopenFlags);
#else // UNITY_STANDALONE_WIN
Copy link
Owner

Choose a reason for hiding this comment

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

Is the way it was somehow bad? I understand the addition of || UNITY_EDITOR_XXX (which should also go there), but not the commented else. I mean, if for any reason the running platform happens not to be listed here, the code should not even compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though it might be useful in some cases as there's quite a lot of platforms. But then it probably makes most sense to leave it as #elif UNITY_STANDALONE_WIN || UNITY_EDITOR_WIN as this is what is directly supported. I will change it to that.

scripts/Editor/MCpiroman.UnityNativeTool.Editor.asmdef Outdated Show resolved Hide resolved
scripts/DllManipulator.cs Outdated Show resolved Hide resolved
scripts/DllManipulator.cs Outdated Show resolved Hide resolved
scripts/Editor/DllManipulatorEditor.cs Show resolved Hide resolved
@mcpiroman mcpiroman merged commit f4eb21d into mcpiroman:master May 1, 2020
@mcpiroman
Copy link
Owner

Thanks

@rogerbarton
Copy link
Contributor Author

rogerbarton commented May 1, 2020

Thanks for reviewing!! I hope its not too much of a pain. I've got two small PRs left and then that should be it :D

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

2 participants