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

Support for Scripting Implementation per Platform #91

Merged
merged 16 commits into from
May 12, 2023

Conversation

RobProductions
Copy link
Contributor

I've been interested in this tool since it was pretty similar to some custom scripts I wrote in a past project that dealt with automating builds. I was pretty impressed when I tried it out, but one thing necessary for my games is the ability to specify Scripting Implementation per build. I really only wanted this because I have some high performance operations that may take better advantage of IL2CPP as opposed to Mono.

After seeing that you had an Issue which talks about this, I figured everyone should get the benefit of this option which I really wanted to see from this build. So I went ahead and added support for Scripting Backend as an extra delimiter for builds, and here's a summary of the changes I made for this PR:

Scripting Backend Options

With these changes, you're now able to see extra options in the PC Platform like this:

image

I tried as best possible to follow the patterns and conventions laid out in the code that I saw, so I made a new class called BuildScriptBackend which is used much like the BuildArchitecture class throughout the project. Note that from my understanding and testing in past projects, PC is the only platform on which IL2CPP is available, and the rest use Mono. There was a 3rd option I saw when looking through the Scripting Implementation enum, but I'm unfamiliar with it and didn't know which Unity version it was introduced in. Adding it would be pretty simple though; just one line in the PC platform just like you've structured the architectures. It would also be simple to add in IL2CPP for other platforms if support was confirmed for them.

Back to the UI, this new Script Backend option will now show up in the config tree. Though it's not common, I figured it was better to give users the option to compile for both IL2CPP and Mono if they want. As a result, if both are enabled you can get both as separate builds:

image
image

If no Script Backend option is selected, the build will disappear from list. This is identical behavior to what happens when you don't select an architecture:

image

As you can see, currently the other platforms remain unchanged. Consequently, the code is able to detect when the "script backend" list is empty and will use a fake "default" script backend which uses Mono 2x Runtime. This is a similar approach to when there's only one architecture available and requires less specifying per platform. You can see this in action in the Defines list when selecting a Platform with no backend options:

image

And here's what it looks like with a specific Script Backend on PC (note the new info panel too):

image

Since this will result in different types of builds, I added a new Build Path token called "$BACKEND" which works to differentiate the file path like other tokens. Upon build, the scripting backend info gets sent to the Configure Environment function and it performs the switch like this:

image

To confirm that it works I ran the tool which created 2 builds and used Unity's own compile time constant:

#if ENABLE_IL2CPP

which detects if IL2CPP is enabled. Here is the resulting output path which now takes into account the backend token:

image

I used IMGUI to print text to the screen based on the keyword and this is what I got for the IL2CPP variant build:

image

Then for the Mono 2x Build:

image

This should be proof that it functions as expected but feel free to try it on your own! I think this sort of flexibility is really nice and fits with the design of the other features. You can now create tons of different types of builds on PC if you wish:

image

Other changes

I made a few other QOL changes which I'll briefly mention. For one, I was having issues with the Truncate function as it seemed to not correctly handle more than 3 forward slashes. This became a problem now that the Build Configuration title had an extra slash due to the scripting backend option. So instead of adjusting Truncate, I thought it would be better to put some UX design principles forward by removing the height constraint entirely and letting long strings wrap to the next line. I find it much less obstructive than trying to limit the text and for most cases I imagine users will never see the second line anyways. Here's a screenshot of that in action:

image

Aside from that, I also adjusted the package JSON file to add some missing metadata like "author" and I put SuperUnityBuild as the author. I did this because when you import via the Package Manager you'd now be able to see the extra details/sorting options which makes it look nicer compared to the builtin packages.

Apologies for the long post, but I hope these changes are beneficial to everyone using the package, and feel free to give feedback if you'd like anything changed. I didn't edit the version number of the package JSON yet but if you'd like me to do that too I can. I've got a few other QOL changes in mind for the UI but I wanted to submit this first. Thanks!

Fixes #87

@robinnorth
Copy link
Collaborator

Hi @RobProductions, thank you very much for your PR and detailed breakdown of your changes. Adding Scripting Backend selection to release types has been an aim of mine for a while (hence #87), so this is a very welcome contribution indeed!

I'll take an in-depth look at everything later and do a proper code review, but for now I will just say that IL2CPP is in fact available for all of the other platforms that SuperUnityBuild supports -- Android, iOS, macOS (requires a macOS device to build), Linux, PC, UWP and WebGL -- see https://docs.unity3d.com/Manual/ScriptingRestrictions.html. Additionally iOS, UWP and WebGL only support IL2CPP and not Mono. Would you mind extending this PR to support those requirements in the meantime? You may need to install some additional build platform modules for Unity to test this. I regularly build for iOS and Android as well as PC so can help with testing these changes.

@RobProductions
Copy link
Contributor Author

@robinnorth Gotcha, I can do that. Sorry about the poor assumptions haha I was basing my information off of my discoveries in this post I made at the Unity forums, where I got the error:

Error building Player: Currently selected scripting backend (IL2CPP) is not installed.

When I tried to run BuildPipeline for Mac builds from my PC. So I'm guessing that's what gets thrown when you're not using a Mac Editor for IL2CPP? I can probably do more research on this and use the code from this thread to detect IL2CPP availability. Unfortunately I can only test building from PC but I will try to run the exports for most of those other platforms to at least make sure they succeed. Thanks!

@RobProductions
Copy link
Contributor Author

RobProductions commented Nov 23, 2022

Sadly I've hit a bit of a crossroads on this. I was able to add in an IL2CPP detector and use it in the Init() function like this:

image

And it worked to distinguish between IL2CPP installation as you can see here:

image

I'm even able to see Linux because I have the Linux IL2CPP module installed:

image

However when I tried to run it on a module that wasn't installed (UWP for me) it seems that the GetPlaybackEngineDirectory function internally calls Debug.LogError or something like that. I get this log in the console:

Build target directory for MetroSupport does not exist!

It's not a catchable exception and I can't seem to suppress the Log in any way. Using Debug.unityLogger.loggingEnabled = false doesn't seem to have an effect on it. Instead I tried to detect if the Module is installed before getting to that function like this:

if (!BuildPipeline.IsBuildTargetSupported(TargetGroup, Target))
            {
                //Module not installed
                return false;
            }

But that also throws an error due to it being called from a ScriptableObject constructor. I guess there's something that Unity BuildPipline doesn't like about ScriptableObject initialization and it won't let me detect the current installed modules even if I try other functions like that one. In the end, the IL2CPP detector works properly (I can still tell when it failed due to the result of the function which is null or empty string and just return false) but I can't seem to get rid of that LogError which is definitely not acceptable for users to see when they run the package.

So I see a few paths ahead here:

  1. Keep researching and find a way to suppress the Log OR
  2. Find a way to detect installed modules and avoid calling the function in question OR
  3. Don't use IL2CPP detection at all and trust the user to know which Scripting Backends they can use for their project. This is somewhat similar to the fact that the package allows you to select UWP as a Platform even when the UWP module is not installed. From there I imagine there will be an error if you try to build UWP which will let the user know what went wrong. So for this I could just plug in both backends on the supported platforms and let it fail when the user tries to build. The only concern is simply whether users will be too confused by having this option which won't work on build, but again you're already expecting users to know what Platform modules are installed so I don't see it as a huge leap

I'm not leaning any particular way on this but just wanted to give an update, so let me know if you have a preferred approach. I'll push my progress so far but it's not ready for merge just yet. I could put things together for Option 3 for now if you'd like it more immediately and can look into the other 2 options for the future; that might be the best way forward for now?

@RobProductions
Copy link
Contributor Author

@robinnorth Sorry it's been a while, I wanted to clarify my question: would you like me to prepare the branch for Option 3 as specified in my last comment? I could do it with one more quick commit and after that I'd say it's ready for merge if you approve the changes. Thanks for your time!

@robinnorth
Copy link
Collaborator

Hey @RobProductions, I think Option 3 sounds fine to me. As you say, users are expected to know which Build Platforms are available. Detection of scripting backends could always be added as a future enhancement, but for now I think even just having the ability to specify per-platform scripting implementations is a big win. Cheers!

@RobProductions
Copy link
Contributor Author

RobProductions commented Feb 17, 2023

@robinnorth sounds good, thank you! I've now updated the PR with the changes, so now there's no IL2CPP detection and the following platforms all have IL2CPP options: Windows, UWP, Mac, Linux, iOS, Android, and WebGL as can be seen here

image

I also tested what happens when you try to build for IL2CPP on a target platform that's not supported and the user will get this error from BuildPipeline:

image
image

So I believe that's a good enough message indicating that the user should correct the settings and I'm happy with this solution for now!

I believe the PR is ready for review so feel free to check it out and merge it if you approve the changes; also feel free to test the IL2CPP implementation yourself on various platforms. As mentioned before I was only able to confirm it working on Windows but due to the message I got exporting to Mac & the same code being run on both I'll assume it's fine for everything else provided the user has the module installed & supports it.

I do have other ideas for further UI enhancements/features/cleanup as mentioned originally but I'll save it for another PR in the near future. Thanks again for helping to get this fleshed out :)

@RobProductions
Copy link
Contributor Author

Sorry, one last thing. Somehow I totally missed this when you said:

Additionally iOS, UWP and WebGL only support IL2CPP and not Mono.

In your last comment. So for these platforms I actually included the "option" for both as I showed in my screenshot. However, just as precursory search I did find this about iOS the Unity docs for unity 2018:

image

So while it may not be accepted by the App Store(?) Mono builds are still possible(?)

Whereas newer versions of the iOS settings page say this:

image

So it seems in Unity 2019 or 2020+, this setting may have been adjusted. Not sure if it's worth it to keep the option for both or if it even matters as Unity may just override what you set, but I can't say because I haven't tested it. Any insight on this?

Also for Android, even in the 2023 version of Unity, the docs seem to have no restriction on Mono builds:

image

Same for WebGL, although this screenshot seems to suggest otherwise as it's greyed out:

image

So uh... sorry for missing that. I'm thinking it's worth it to keep the option for both in Platforms where it's still possible, even if only for Unity 2018. But I'm not actually sure since I haven't installed those Platforms recently and not sure if Unity will actually just export with the correct backend anyways no matter what you set (since the user option could be greyed out). Worth testing I suppose so let me know if you have any suggestions. It would only be a few lines changed in those platforms if anything needs adjusting.

@RobProductions
Copy link
Contributor Author

RobProductions commented Feb 24, 2023

@robinnorth Apologies again for writing so much and being confused about this, but I have to say it's not easy to do much more without more information or feedback. Unity's docs are a nightmare as usual haha and I'm not too keen on bloating up my older Unity installs with a bunch of modules to test this. I've tried my best to look into which platforms support which backends and even loaded up an old WebGL project to see what was going on and confirmed that at least for WebGL on Unity 2019, the backend may not matter as I just get "Default":

image

Meaning even if buildtool tries to change the scripting backend, I think Unity will just override it with whatever it supports. Still, it's nice to have the labels match when possible. So I've made a best guess effort at support levels and these are the option changes I made in my latest commit just now:

  1. iOS is only given the IL2CPP backend. From my research it may have been the case that older versions of Unity could export to iOS Mono, but those builds won't work on iOS version 11 and up which I believe is already pushing past the level of support of the App Store anyways so you wouldn't realistically be able to use builds like that nowadays.
  2. Android has both Mono and IL2CPP, but IL2CPP is now the default option. It seems even in modern versions of Unity, Mono builds for Android could still be supported, but not recommended. So I'm leaving Mono as an option just in case.
  3. WebGL is only given the IL2CPP backend. From my understanding in Unity 2019 it seems IL2CPP is the "default" option. Not sure about older versions but I'll guess that it was like this before 2019 too.
  4. Universal Windows Platform gets IL2CPP and the outlier .NET Runtime. I don't see .NET Runtime listed on the newest Unity 2023 doc page, but it is present on most of the others. Worst case this could become an error in future versions if they remove the enum value, but as of Unity 2020 it's not even listed as deprecated yet so I'll leave it. In the future if someone confirms its deprecation on 2023 I could detect unity version there and remove it from 2023 and up.

It's taken quite a bit of stumbling to get to this point so that will be my last commit here unless any other changes are necessary. I'll once again declare this ready for review until anything else pops up. I'm hoping this can get merged within the next few weeks because I'd like to shift focus to further enhancement PRs and actually prepare to use this for my game. Thanks for the help again and let me know if everything looks good.

Edit: And in case it wasn't clear, the reason I struggled trying to research instead of just referencing the doc link you provided was 1. to confirm it because Unity docs can sometimes be inaccurate in my experience and 2. to understood support levels across previous Unity versions and not just the latest LTS

@robinnorth
Copy link
Collaborator

Hi @RobProductions, thanks so much for all of your efforts and detailed research on this, I really appreciate it! I will set aside some time to properly review all of this with a view to getting it merged for you ASAP.

@robinnorth
Copy link
Collaborator

@RobProductions a quick update on this, I had a chance to pull and review your changes but realised that the Build Actions system needs to be updated to work with this as well, given the changes to some of the method signatures this PR makes. I made a start on that, but haven't gotten around to finishing it just yet. Unfortunately that's currently blocking the merge, but otherwise everything else looked good to me!

@RobProductions
Copy link
Contributor Author

@robinnorth Great thanks for taking a look! My bad for missing that, I never really used the actions before so I didn't consider it could be a problem too. Feel free to push whatever edits you have to the PR (I think that's possible? Never tried it before honestly but this seems to suggest its a feature) and let me know if you'd like any help looking into it :) happy to contribute whatever I can if anything ends up being a complicated block

@robinnorth
Copy link
Collaborator

@RobProductions I've just pushed a couple of commits to the PR (thanks for enabling that, it worked seamlessly), one which updates the Build Actions system to be aware of the scripting backend for a build, and one which removes the .NET backend for UWP, as it turns out this was deprecated in Unity 2018.2: https://forum.unity.com/threads/deprecation-of-support-for-the-net-scripting-backend-used-by-the-universal-windows-platform.539685/

The only remaining task to do before merging this all in now is to test in the various supported Unity versions, which I will try and do ASAP. Thanks again for your patience on this!

@RobProductions
Copy link
Contributor Author

@robinnorth Awesome looks great, thank you for double checking that!

In the meantime I've been thinking about the UI improvements I mentioned before and I wanted to make sure some of my ideas sound okay with the project maintainers haha. Most of them would be simple changes in placement/buttons in the buildtool window that I would just submit via PR, but a few of them have to do with the UX aspect and the behavior of the package interacting with user's projects. So in that regard I wanted to make sure the suggestions would be accepted before actually doing the work of switching it up. Where do you think is the best place to discuss that? Posting in the Github issues? Thanks again :)

@robinnorth
Copy link
Collaborator

@RobProductions UI/UX suggestions would be most welcome indeed, opening new issues to track them would be the best way to handle discussion around those, I think. Looking forward to hearing your suggestions -- I have long thought the tool needs a bit of UX attention, but have not had the time to invest in that myself as it works well enough for my use of it in my day job 😅

@robinnorth
Copy link
Collaborator

Hi @RobProductions, I've just been spending a bit of time reviewing this PR in more detail and have realised that there is one more change that needs to be made before it can be merged, after all.

Unlike BuildArchitectures, on which I presume the BuildScriptBackends implementation has been based, a BuildPlatform should only have one script backend configured, as PlayerSettings.SetScriptingBackend will only accept a single ScriptingImplementation. At present, the use of an array of backends and corresponding UI check boxes to select them would allow a user to either enable more than one backend (or none at all!). Instead, this feature should act like the BuildVariants functionality where the user is presented with a dropdown list of available backends for the platform, thus permitting only one to be selected, like the 'Device Type' selector here:

image

Would you be able to implement this as described?

@RobProductions
Copy link
Contributor Author

Hi @robinnorth , forgive me if I'm a bit rusty on the code as it's been a few months but I believe I intended it to work this way as I posted in my original PR comment:

Though it's not common, I figured it was better to give users the option to compile for both IL2CPP and Mono if they want. As a result, if both are enabled you can get both as separate builds

And then I posted a picture of the config tree which shows both variants counting as seperate configs to be marked for build. Now the reason why I believe this should work is because SetScriptingBackend is called from ConfigureEnvironment() as you can see here:

image

Which is called from BuildPlayer:

image

Which I thought gets called for each config that's listed in the config tree with variables populated via its keychain:

image

So I might be totally wrong lol and let me know if I'm forgetting something. But to me it seems like since the configuration happens for each build it doesn't matter that SetScriptingBackend only takes 1 backend because it's being handed that value and re-run before each build starts. i.e. if a user selects both backends for a BuildPlatform, it will result in 2 distinct keychains and thus 2 distinct configs which are sent to BuildPlayer individually. I thought this was something I accounted for by adjusting the keychain and I think I tested this scenario too but let me know if you get a different result.

Thanks and let me know what you think :)

@robinnorth
Copy link
Collaborator

My apologies, @RobProductions, you are correct and this does produce separated configuration keychains for each selected backend. I didn't think I was seeing this happening when I was reviewing it yesterday, but have just tested again and can see that this is in fact the case:

image

Disregard my previous comment, I'll continue with reviewing and testing this and then we can get this wrapped up!

@RobProductions
Copy link
Contributor Author

@robinnorth No worries! If it's a design concern instead of just a functionality thing I'm happy to discuss updates to the way the option is shown/rendered, but personally as a user I like it the way it is since it provides the most options. As I mentioned it's likely rare to want more than one Scripting Backend, but it doesn't hurt to include it just in case.

I appreciate the cleanup on this branch too since I had some of the same ideas you just implemented haha. Looks great! Let me know if you have any other suggestions and once it's merged I'll start looking into the UX changes I mentioned, thanks!

@robinnorth
Copy link
Collaborator

@RobProductions absolutely not a design concern at all, I think it's a really useful addition to the library and I certainly have been in the position in the past where I'd have liked an easy way to build for both backends at once, so am very happy with how it's been implemented.

You're welcome regarding the cleanup, I thought it would just be quicker and easier to make the changes rather than asking you to do even more work on this PR. Glad you don't mind me doing so!

Should be nearly ready to merge now, I just want to do a little more testing first.

@robinnorth robinnorth merged commit c2b53e3 into superunitybuild:dev May 12, 2023
@robinnorth
Copy link
Collaborator

Wrapped on my testing and merged! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants