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

feat: preserve property Id from asset data #313

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

ericserrat
Copy link
Contributor

@ericserrat ericserrat commented Aug 26, 2024

SRV-1333

Description

  • Preserve the Id from the IAsset interface, so when stripped in mobile it does not remove it

How to Test

  • Build a sdk version for android using stripping level high, try to run it in the android emulator

@ericserrat ericserrat requested a review from a team as a code owner August 26, 2024 16:33
Copy link
Collaborator

@HarrisonHough HarrisonHough left a comment

Choose a reason for hiding this comment

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

Nice work on this!
However, I would actually suggest putting the [Preserve] attribute on the whole class rather than just on the Id property to make sure that everything gets preserved.
EG

using UnityEngine.Scripting;

namespace ReadyPlayerMe.AvatarCreator
{
    [Preserve]
    public interface IAssetData
    {
        public string Id { get; set; }
        public AssetType AssetType { get; set; }
    }
}

@HarrisonHough
Copy link
Collaborator

We could alternatively to make use of a linker file instead of using the preserve attribute. When using a link file you can preserve an entire namespace. EG something like this

<linker>
  <assembly fullname="Assembly-CSharp">
    <namespace fullname="ReadyPlayerMe.AvatarCreator" preserve="all"/>
  </assembly>
</linker>

This would preserve every class that is a part of ReadyPlayerMe.AvatarCreator, this may be overkill but it might prevent weird issues where certain stripping settings will strip random parts of our SDK code.

Overall as long as the preserve tag works it doesn't really matter which approach. We should in any case make sure QA does thorough testing of this with different stripping settings on different platforms to ensure it works.

@ericserrat
Copy link
Contributor Author

Nice work on this! However, I would actually suggest putting the [Preserve] attribute on the whole class rather than just on the Id property to make sure that everything gets preserved. EG

using UnityEngine.Scripting;

namespace ReadyPlayerMe.AvatarCreator
{
    [Preserve]
    public interface IAssetData
    {
        public string Id { get; set; }
        public AssetType AssetType { get; set; }
    }
}

Yep, I will prefer it like this, but tried it, and did not work.

@ericserrat
Copy link
Contributor Author

We could alternatively to make use of a linker file instead of using the preserve attribute. When using a link file you can preserve an entire namespace. EG something like this

<linker>
  <assembly fullname="Assembly-CSharp">
    <namespace fullname="ReadyPlayerMe.AvatarCreator" preserve="all"/>
  </assembly>
</linker>

This would preserve every class that is a part of ReadyPlayerMe.AvatarCreator, this may be overkill but it might prevent weird issues where certain stripping settings will strip random parts of our SDK code.

Overall as long as the preserve tag works it doesn't really matter which approach. We should in any case make sure QA does thorough testing of this with different stripping settings on different platforms to ensure it works.

As discussed in huddle, seems that this is also not working for us.
For now we're going to leave it with the [Preserve] in the property

@HarrisonHough HarrisonHough self-requested a review August 27, 2024 09:03
Copy link
Collaborator

@HarrisonHough HarrisonHough left a comment

Choose a reason for hiding this comment

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

Thanks for doing that, weird that the other approach didn't work though.

All good though.

@ericserrat ericserrat changed the base branch from main to develop August 27, 2024 11:24
@ericserrat ericserrat merged commit 2879f7a into develop Aug 27, 2024
5 of 8 checks passed
@ericserrat ericserrat deleted the feature/SRV-1333-stripped-template branch August 27, 2024 11:25
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