search

adjust / unity_sdk

This is the Unity SDK of
http://www.adjust.com
MIT License
149 stars 71 forks source link

Asmdefs are missing #221

Open recstazy opened 3 years ago

recstazy commented 3 years ago

Hello there! It would be very nice to have the sdk asmdefs to be able to reference to Adjust namespaces from other assembly defines. For now it's only possible to use Adjust from main assembly. Also missing asmdefs forces unity to recompile adjust every time scripts are recompiled. Are you planning to fix that or we can just forget about it?

MatkovIvan commented 3 years ago

Hi there,

Thanks for getting in touch. It's in "nice to have" section in our backlog, but we cannot provide any ETA yet. For everyone else who interested in it: please add "👍 " reactions so we can see the interest and properly prioritize it along other feature requests.

Thanks, Ivan

recstazy commented 3 years ago

Oh, thank you very much for a reply, nice to see that support is alive!

ilterbilguven commented 3 years ago

I think it's more than "nice to have" nowadays. Also, it's quite easy to add it.

lolerji commented 3 years ago

If you are distributing code with Unity's Package Manager, assembly definition is a "must", not "nice to have". Honestly I can't really see why this is in backlog or is being delayed, adding it shouldn't be a hard thing.

uerceg commented 3 years ago

Hey guys. We are currently simply working on other things which we plan to release soon and this topic didn't make the list yet. We are definitely gonna take a look at this as well afterwards (together with Unity Package Manager which we still didn't build support for, thus no asmdefs). We'll keep you posted.

revolt3r commented 2 years ago

It literally takes a few minutes to create the Assembly Definitions. Right now you're forcing devs to modify the sdk files, which makes it more complicated to update it every time.

ilterbilguven commented 2 years ago

Hey guys. We are currently simply working on other things which we plan to release soon and this topic didn't make the list yet. We are definitely gonna take a look at this as well afterwards (together with Unity Package Manager which we still didn't build support for, thus no asmdefs). We'll keep you posted.

Well, it's been more than one year.

uerceg commented 2 years ago

AdjustOaid_v4.31.0_asmdefs.unitypackage.zip Hey guys,

And yes @ilterbilguven, indeed. Sorry about that, not really proud because of such a big delay.

We managed to check this topic and "thanks" to our current code organization, only asmdef files which are possible to be generated w/o actually need to reshuffle things inside of the package are the AdjustEditor.asmdef and root Adjust.asmdef file. I do understand that this might not be something everyone wanted, in our next major (SDK v5) update we will take this topic into consideration and try to organize things hopefully in better way to allow split into more assemblies if needed.

Before we make this part of our official release, it would be great if someone would be willing to test current latest SDK version just with asmdefs added to it and also package.json file in order to add support for UPM. State of code which was used to generate these .unitypackage files can be found in asmdefs branch.

Adjust_v4.31.0_asmdefs.unitypackage.zip AdjustImei_v4.31.0_asmdefs.unitypackage.zip AdjustOaid_v4.31.0_asmdefs.unitypackage.zip

Also, in case someone is willing to try whether UPM integration works well and let us know, that'd be great as well before officially pushing things to master branch. Since I didn't manage to instruct to UPM to add package from specific Git branch URL + to specify path there, I have added state of above mentioned asmdefs to master branch of my private fork of the unity_sdk repo. In order to add our Unity SDK to your app:

Looking forward to hearing back from you guys and one more time - apologies for the delay.

Cheers

marcos-monumental commented 1 year ago

@uerceg Hi, I tried out this UPM integration. There were some changes that we needed to make to make it compatible with our projects, so here's my exact steps to get things working:

  1. Forked the repo (I was asked to make this private)
  2. Merged the asmdef branch into master to get latest with asmdefs, this was a clean merge
  3. Json.net DLLs included in the repo conflicted with references already in our project, this is a common pain point that others will likely hit: a. Deleted /Assets/Adjust/Test/3rdParty and .meta b. Deleted /Assets/Adjust/Windows/Newtonsoft.Json.dll and .meta c. You could try adding com.unity.nuget.newtonsoft-json as a package dependency, but this probably won't fix the issue for everyone, I am not sure if there's a perfect solution to this admittedly
  4. AdjustSettings.cs creates the AdjustSettings.asset scriptable object instance in the same folder as the script. This is a problem because the contents of packages are immutable. It fails to create a meta file due to this immutability, so Unity ignores the file. Furthermore the immutabality means that you can't actually change those settings.

To get around this immutability issue I reworked the scriptable object instance creation to create and load the file from /Assets/Editor/Adjust/AdjustSettings.asset instead of within the package.

You can see all my changes in this demonstration PR I created https://github.com/adjust/unity_sdk/pull/256, although you can obviously disregard the readme changes. (The forked repo has since been deleted and made private)

I did not test IMEI or OAID.

uerceg commented 1 year ago

Hi @marcos-monumental,

Thank you very much for your feedback and such a detailed checks and sorry for a bit delayed response. We will try to take a look at everything you mentioned in days to come and ping you with answers / follow up questions. Big thanks one more time. 🍺

marcos-monumental commented 1 year ago

Hey @uerceg,

Had another issue come up today when I finally got around to building for iOS, basically running into the issue described here: https://github.com/adjust/unity_sdk/issues/172 with iOS failing to build due to missing bitcode for test related files

Using the unity package as reference, I deleted Adjust/Test, Adjust/Android/Test, Adjust/iOS/Test, and Adjust/Windows/Test. The iOS build is in progress now but I expect it should now succeed.

hanaytug commented 1 year ago

Hey guys. We are currently simply working on other things which we plan to release soon and this topic didn't make the list yet. We are definitely gonna take a look at this as well afterwards (together with Unity Package Manager which we still didn't build support for, thus no asmdefs). We'll keep you posted.

Well, it's been more than 2 years.

uerceg commented 1 year ago

Hey @hanaytug,

Since the issue has been opened, true. But less than that since @marcos-monumental tried out the suggested POC which I wanted to say many thanks. Unfortunately, we had some more urgent things in backlog lately, but we do aim to close this topic in Q3.

uerceg commented 10 months ago

Hey guys. Just wanted to ping here for an update that this task has been shifted a bit on our end due to post-WWDC2023 tasks that got prioritized on our end and is aimed to be done in October. I'll keep you posted.

uerceg commented 10 months ago

Okay, one more time sorry for delay on this one and thanks for all the patience and feedback.

@marcos-monumental big special thanks to you and in this comment of mine, I'm looking in your direction and things that you have mentioned in this comment.

Json.net DLLs included in the repo conflicted with references already in our project, this is a common pain point that others will likely hit: a. Deleted /Assets/Adjust/Test/3rdParty and .meta b. Deleted /Assets/Adjust/Windows/Newtonsoft.Json.dll and .meta c. You could try adding com.unity.nuget.newtonsoft-json as a package dependency, but this probably won't fix the issue for everyone, I am not sure if there's a perfect solution to this admittedly

Yes, this is an interesting issue, not sure still how to dance around it in some optimal way. However, in the current solution I think we can have as an initial way to go for UPM usage, I have removed completely all the testing related things because other than helping us internally with the testing, there's no much gain for clients in having these files inside of their apps - it's completely useless. According to what you wrote and faced as an issue, it seems to me that Newtonsoft.Json.dll from Windows directory might still be an issue though. We'll try to look into solving this maybe in some better way.

AdjustSettings.cs creates the AdjustSettings.asset scriptable object instance in the same folder as the script. This is a problem because the contents of packages are immutable. It fails to create a meta file due to this immutability, so Unity ignores the file. Furthermore the immutabality means that you can't actually change those settings.

To get around this immutability issue I reworked the scriptable object instance creation to create and load the file from /Assets/Editor/Adjust/AdjustSettings.asset instead of within the package.

Thank you very much for this and the contributions. I have cherry picked your commits and made them part of this initial UPM support version.

If you (and anyone else following the ticket) would be willing to try adding the Adjust Unity SDK via UPM from https://github.com/adjust/unity_sdk.git?path=/Assets/Adjust#upm and let us know how does that work for you, that would be lovely.

Sorry one more time for the waiting and looking forward to hear back from you.

ErnSur commented 6 months ago

Hi @uerceg, at Homa Games we are migrating all of our publishing SDK packages to UPM format. Since we are using Adjust we'll need to do the same with your SDK. Thankfully I found your post before doing any work 👍 We'll be testing the upm version of the package from your upm branch, https://github.com/adjust/unity_sdk.git?path=/Assets/Adjust#upm.

I'll come back here in the future to leave our feedback. Thanks!

uerceg commented 6 months ago

Hi @ErnSur,

Thank you very much for that! Looking forward to hearing feedback from your end.

Cheers

ErnSur commented 6 months ago

@uerceg After one fix I was able to build a project. Fix is here -> #286

I also saw that there's a Newtonsoft.Json dll in the Windows folder. We don't care about this platform but ideally you should probably replace it with a package dependency to the official newtonsoft unity package. Like this:

// in package.json
"dependencies": {
    "com.unity.nuget.newtonsoft-json": "3.2.0"
  },
ilterbilguven commented 1 month ago

@uerceg I see that Adjust v5 is coming, but it still has not asmdefs. I'm tired of downloading, converting to UPM every single update, which I have been doing for at least 3 years. Please prioritize this and #259. So, we can use it with OpenUPM and EDM4U without any external modification.

uerceg commented 1 month ago

Hey @ilterbilguven,

That's right, asmdefs are still not there in the v5 beta (sorry about that), but we're planning to add them before the v5 is made official. For the time being, it's just structured in a way how v4 is, but once live, it should come with asmdefs.

uerceg commented 1 month ago

@ilterbilguven Oh, right and also for the time being (v5 beta) we are sticking with native dependencies as iOS framework and Android AAR, because we don't have native versions officially pushed to public CocoaPods and Maven channels, but once those are live and once we are good to release Unity v5 officially, we're gonna swap those in favor of External Dependency Manager. Also thinking in direction of packaging EDM together with our SDK's .unitypackage. Maybe on that topic - how does the packaging both together sound to you? Would it be something that'd intrude with your packages adding flow or not?

ilterbilguven commented 1 month ago

@uerceg for supporting older Unity versions, which don't have UPM, it's fine.

For a more modern approach, it needs to be in UPM format. You can put the SDK under Packages folder instead of Assets, and still make it a .unitypackage.

Google officially supports EDM4U on OpenUPM, you can add it as a dependency and release the SDK to OpenUPM, which will make everything sooo simple.