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

Removing UnityEngine reference inside com.rlabrecque.steamworks.net.asmdef #630

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

coty-crg
Copy link

To improve compile times, I've removed the UnityEngine from the base of the plugin. The one reference was simply to pass along exception logging, so I've instead replaced it with a static delegate. In SteamManager.cs, you can hook into this delegate in OnEnable()/OnDisable().

I've attached some before and after examples of how compile times an be affected by this dependency chain change.
Unity_T36hsZdink
Unity_tFU2GfQlTM

…t.asmdef

To improve compile times, I've removed the UnityEngine from the base of the plugin. The one reference was simply to pass along exception logging, so I've instead replaced it with a static delegate. In SteamManager.cs, you can hook into this delegate in OnEnable()/OnDisable().
@coty-crg
Copy link
Author

Opened a pull request for the corresponding change for SteamManager.cs here: rlabrecque/Steamworks.NET-SteamManager#7

@rlabrecque
Copy link
Owner

I like this A LOT; I wonder if it's possible to avoid breaking compatibility with this though as It's not really expected that people have to update or change their SteamManager (or equivalent). Might need to think on this one for a minute

@coty-crg
Copy link
Author

Ah, yeah.. I can see how that's a bit of a pain. It's too bad the SteamManager script itself isn't also in a package manager repo, so you could just have the packages in sync via dependencies.

@rlabrecque
Copy link
Owner

Yeah, though it's explicitly not just because it was originally just a place to start from as 'user code'. As the initial users each had unique ways of handling their 'managers', like some didn't have or want that type of code as a MonoBehavior aaat all. Being unopinionated here is the only reason why Steamworks.NET is still around at all.

Trying to use Steamworks.NET via the package manager again this last week for the first time in a year or two was somewhat frustrating because of this though; so I was already having some thoughts about how we could ship SteamManager but with an option (somehow) to copy it into the users Scripts directory (and then also allow people to update their copy if they are using an untouched version). I'm not even quite sure how such an option would interact with this specific change too.

Some other options we have:

  1. Warn somewhere, somehow, if that delegate isn't bound?
  2. Do this in two distinct steps, adding the new callback, and then removing the UnityEngine reference in the future?

@coty-crg
Copy link
Author

Option 2 makes sense to me! Gives people ample time to eventually go to the callback system. I don't want to cause any issues with this pull request, I know how frustrating breaking changes can be.

Having the ability to simply copy in the manager seems good too, although personally I never actually edit that file. I assumed we were meant to inherit from it, and then make any changes necessary.

@rlabrecque
Copy link
Owner

Maybe this code initially for option 2? Also Let's us get the SteamManager change in ASAP too.

		public delegate void OnSteamException(Exception e); 
		public static OnSteamException OnSteamExceptionEvent; 

		public static void ExceptionHandler(Exception e) {
			if (OnSteamExceptionEvent != null) {
				OnSteamExceptionEvent.Invoke(e); 
			}
#if UNITY_STANDALONE
			else {
				UnityEngine.Debug.LogException(e);
			}
#elif STEAMWORKS_WIN || STEAMWORKS_LIN_OSX
			else {
				Console.WriteLine(e.Message);
			}
#endif
		}

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.

2 participants