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

Set functions to modify ScriptableSentryUnityOptions from internal to public #419

Conversation

RiederAlex
Copy link
Contributor

Set functions to modify ScriptableSentryUnityOptions from internal to public

Why?
Unity projects quickly outgrow the features provided by the default "Build" and "Build and run".
What ends up happening is that Unity users begin to script their builds from within via C#, which includes automated changing of various values for scriptable objects - think of information like branch/commithash/timestamp - that should be included in the game, but are only known during buildtime.

This pull requests make this possible for ScriptableSentryUnityOptions, to e.g. set the environment/release per branch or different dsn for production/development.

@bitsandfoxes
Copy link
Contributor

Hey, thanks for opening this up. If I catch your intention: You want to programmatically modify the scriptable options object instead of using the programmatic initialization approach? Are there any features in particular you are missing?
As a workaround to unblock right now you you could overwrite the actual text inside the option.asset

@RiederAlex
Copy link
Contributor Author

Yes, the point being that this happens during player build-time, not during runtime!

To rephrase it bit differently: The CI I'm currently working with calls an editor-only C# function, which does a bunch of stuff before building the player via BuildPipeline.BuildPlayer. A bunch of stuff in this case is things like injecting commit id/branch and including/excluding certain scenes. This works by changing the scriptable object(s) referenced in the project and then saving the asset again before building the player, so it gets included into the build.

@bitsandfoxes
Copy link
Contributor

While we do see your point and the use-case we're reluctant to just merge this.
For example: We read from the scriptable object during different times, i.e. IPostGenerateGradleAndroidProject.OnPostGenerateGradleAndroidProject for Android or OnPostProcessBuild for iOS. So if the scriptable object was just public we'd have no way of knowing that it changed and we could end up with settings not making it into the build.
Do you see an alternative solution?
We thought about maybe platform-sensitive settings, a bit like the build settings themselves with the option to override specific ones. Or multiple scriptable objects that get flagged for platforms.

@RiederAlex
Copy link
Contributor Author

I've looked at the code and the hooks you mentioned remain unaffected by this change, as what I want to achieve is happening before these get executed.

So basically it is: <MyStaticC#FunctionWhichPreparesTheBuild> -> BuildPipeline.BuildPlayer (which calls OnPostGenerateGradleAndroidProject/OnPostProcessBuild) -> <MyStaticC#FunctionWhichPreparesTheBuild>
The changes to the scriptable object would happen in the first part, before the BuildPlayer call is made.

Platform-sensitive settings would allow some more flexibility compared to the current system, but
it would still be impossible to have a different environment/dsn per some arbitary settings your SDK does not know of :)

I would also personally advise against going too far in this direction (regarding allowing of configurating everything per platform via the UI), as the simplicity is one of my favorite aspects of this great SDK/product!

@bruno-garcia
Copy link
Member

I've looked at the code and the hooks you mentioned remain unaffected by this change, as what I want to achieve is happening before these get executed.

Sounds like there's no other reason to keep them private? Surely they can be modified outside of our code which includes validations etc, but at that point "You must know what you're doing".

@bruno-garcia
Copy link
Member

We wanted to collect requirements for changes y'all do in such way to perhaps offer first class features to allow those. By making it public it makes it less likely for users to reach out with such requirements. But we can live with that

@bitsandfoxes
Copy link
Contributor

Hi @RiederAlex! Would it be possible for you to rebase this fork onto main? Or pull main into it so we can let CI pass and then merge it?
Thanks a lot!

@RiederAlex RiederAlex force-pushed the feature/scriptablesentryunityoptions_public branch from fdc80af to 91604fe Compare December 6, 2021 19:47
@bitsandfoxes bitsandfoxes merged commit 9d0bc3b into getsentry:main Dec 7, 2021
@bitsandfoxes
Copy link
Contributor

Hey @RiederAlex, I took the liberty of just merging your PR so your contributions would make it into the 0.8.0 we just released. Thanks a lot for contributing!

@RiederAlex RiederAlex deleted the feature/scriptablesentryunityoptions_public branch December 10, 2021 11:04
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.

3 participants