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

Fix project settings resetting on domain reload #678

Merged
merged 4 commits into from
May 30, 2024

Conversation

berniegp
Copy link
Contributor

@berniegp berniegp commented Mar 9, 2024

This resolves #524 by avoiding to needlessly save GvhProjectSettings.xml on every domain reload.

In all 3 modified files, the line

VerboseLoggingEnabled = VerboseLoggingEnabled;

does 3 things on domain reload because of [InitializeOnLoad]:

  1. Loads GvhProjectSettings.xml if not already done
  2. Writes GvhProjectSettings.xml unconditionally
  3. Sets logger.Level

This PR removes step 2) which is unneeded and the cause of #524.

It's potentially possible that writing GvhProjectSettings.xml as a result of user interaction might still in some cases have a race condition. I didn't investigate this. However it's much less problematic than intermittent problems on domain reload.

Why the problem happens

The current code is correctly protecting critical sections, including saving GvhProjectSettings.xml, with lock (classLock). However a problem occurs with parallel asset importing enabled in Unity. In this case there are other Unity Editor processes aside from the "main" one that take care of parallel imports. These other processes each have their own separate instance of classLock.

This results in multiple processes reading and writing to GvhProjectSettings.xml on domain reload. Eventually one of them reads the file while it's being written and ends up with empty settings from Google.ProjectSettings.Load which it then re-saves. As the screenshot in #524 shows, when corruption happens it's always one or more VerboseLoggingEnabled that remain in the settings.

About asset import worker processes

The command line of asset import worker processes looks like:

"C:\tools\Unity-Hub\2022.3.19f1\Editor\Unity.exe" "-adb2" "-batchMode" "-noUpm" "-name" "AssetImportWorker7" "-projectPath" "C:/dev/unity-project" "-logFile" "Logs/AssetImportWorker7.log" "-srvPort" "64205"

The -adb2 argument is unique to asset import worker processes so could be used to identify them as needed.

Build on Windows

Binaries here https://github.com/berniegp/unity-jar-resolver/releases/tag/v1.2.179

I could not build the plugin as it stands. The (reverted) commit ff03ffd has the changes I needed to do to build.gradle.

As far as I can see, the build process prefers Unity 5.6. This version (obviously) isn't supported on Apple Silicon so I tried on Windows.

Steps for successful build:

  1. Install Unity 5.6 with Android and iOS support
  2. Start Unity 5.6 to sort out activation
  3. Apply ff03ffd
  4. Download and extract https://download.java.net/java/GA/jdk11/9/GPL/openjdk-11.0.2_windows-x64_bin.zip to c:\tools
  5. As administrator (so Python installs correctly, but pollutes add/remove programs) build with:
$env:JAVA_HOME='C:\tools\openjdk-11.0.2_windows-x64_bin\jdk-11.0.2\'; .\gradlew build -PUNITY_EXE=’<PATH_TO>\Unity-Hub\5.6.7f1\Editor\Unity.exe’; Remove-Item Env:\JAVA_HOME

Not sure if I'm doing something wrong regarding the build?

Copy link

google-cla bot commented Mar 9, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@berniegp
Copy link
Contributor Author

Update: 1 month in at my org we're not seeing setting file corruption anymore on Windows and MacOS.

@harlan
Copy link

harlan commented May 24, 2024

What are the steps to try to get @berniegp 's #678 tested/merged etc? is that that you need to sign this CLA thing or something?

@berniegp
Copy link
Contributor Author

I already signed the CLA thing, so next steps aren't on me :)

image

@argzdev
Copy link
Collaborator

argzdev commented May 28, 2024

Hey thanks for the PR, let me get some of our engineering folks to take a look into this.

@a-maurice
Copy link
Collaborator

Thanks for the fix, and the detailed write up actually explaining what the issue was, that is definitely a weird case so really appreciate that you tracked it down. I will work on getting a release out with this fix.

As for building it yourself, it can definitely be a bit finicky and depends on a bunch of tools. I wasn't aware of a Unity 5.6 bias, the tests we have run with Unity 2019, but I do know that Apple Silicon can have issues because it depends on a specific python version which isn't available.

@a-maurice a-maurice merged commit 2853d24 into googlesamples:master May 30, 2024
1 check passed
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.

[Bug] GvhProjectSettings.xml changes/resets itself
4 participants