-
Notifications
You must be signed in to change notification settings - Fork 119
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
Conversation
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. |
@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:
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! |
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: And it worked to distinguish between IL2CPP installation as you can see here: I'm even able to see Linux because I have the Linux IL2CPP module installed: 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:
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:
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:
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? |
@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! |
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! |
@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 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: 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 :) |
Sorry, one last thing. Somehow I totally missed this when you said:
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: 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: 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: Same for WebGL, although this screenshot seems to suggest otherwise as it's greyed out: 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. |
@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": 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:
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 |
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. |
@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! |
@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 |
@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! |
@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 :) |
@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 😅 |
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 Would you be able to implement this as described? |
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:
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: Which is called from BuildPlayer: Which I thought gets called for each config that's listed in the config tree with variables populated via its keychain: 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 :) |
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: Disregard my previous comment, I'll continue with reviewing and testing this and then we can get this wrapped up! |
@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! |
@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. |
Wrapped on my testing and merged! 🚀 |
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:
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:
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:
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:
And here's what it looks like with a specific Script Backend on PC (note the new info panel too):
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:
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:
I used IMGUI to print text to the screen based on the keyword and this is what I got for the IL2CPP variant build:
Then for the Mono 2x Build:
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:
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:
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