v0.8.1: Added the experimental VRChat for iOS build support and improved the CI/CD scripts #13
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on updates to CI/CD workflows, configuration files, and Unity assets. A new workflow for push events has been added, along with modifications to existing workflows to include deployment steps for the Changes
Possibly related PRs
Suggested labels
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
Packages/black.kit.toybox/package.json (1)
4-4: Version bump looks good. Consider updating the changelog.The version bump from 0.8.0 to 0.8.1 is appropriate for introducing new features or bug fixes without breaking changes. This aligns with the PR objective of "Version bump to
0.8.1".Don't forget to update the changelog (if not already done) to document the changes introduced in this version.
.github/workflows/push.yml (3)
10-17: LGTM: Job setup and environment variables are well-defined.The job configuration and environment variables are well-structured. Using the latest Ubuntu and defining environment variables enhances maintainability.
Consider adding comments to explain the purpose of each environment variable, especially for less obvious ones like
listPublishDirectory. This would improve readability for other developers working on this workflow.
18-30: LGTM: Initial steps are well-structured and serve clear purposes.The first four steps of the workflow are logically ordered and efficiently implemented. They handle repository checkout, package zipping, .meta file listing, and Unity package creation.
Consider adding a step to validate the contents of the zip file and Unity package after creation. This could help catch any packaging issues early in the workflow.
39-49: LGTM: Caching and package version listing steps are well-implemented.The use of caching for previous run data and the Nuke build command for package version listing are well-structured. The provision of the GitHub token as an environment variable is a good security practice.
Consider breaking down the Nuke build command into multiple lines for better readability. You could use YAML's multiline string syntax to achieve this. For example:
- name: Build Package Version Listing with Nuke run: | "${{ env.pathToCi }}/build.cmd" BuildRepoListing \ --root "${{ env.pathToCi }}" \ --list-publish-directory "$GITHUB_WORKSPACE/${{ env.listPublishDirectory }}" \ --current-package-name "${{ vars.PACKAGE_NAME }}" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}This change would make the command easier to read and modify in the future.
.github/workflows/build-listing.yml (1)
50-51: Approve the addition of global.json deployment step with suggestions for improvementThe new step to deploy the
global.jsonfile is a good addition to ensure its availability in the CI directory. However, consider enhancing its robustness:
- Add error handling in case the
global.jsonfile doesn't exist.- Verify if the file was successfully copied.
- Consider using a more robust file copying method or a dedicated GitHub Action for file operations.
Here's an improved version of the step:
- name: Deploy the global.json file run: | if [ -f "${{ github.workspace }}/global.json" ]; then cp "${{ github.workspace }}/global.json" "${{ env.pathToCi }}/" && \ echo "global.json successfully copied to ${{ env.pathToCi }}/" else echo "Error: global.json not found in ${{ github.workspace }}" exit 1 fiThis version checks for the file's existence, copies it if found, and provides feedback on the operation's success or failure.
Assets/XR/XRGeneralSettings.asset (1)
79-94: LGTM: iPhone Providers section addedThe new iPhone Providers section has been correctly added with appropriate Unity-specific fields. This addition supports the PR objective of adding iOS build support.
Note: The
m_Loadersarray is currently empty. While this may be intentional for the initial setup, consider populating it with relevant loaders in the future if needed for iOS-specific XR functionality.Packages/packages-lock.json (1)
214-215: LGTM: Addition of UGUI dependency enhances cross-platform supportThe addition of
"com.unity.ugui": "1.0.0"to thecom.vrchat.worldspackage dependencies is a positive change. This inclusion of Unity's UI system aligns well with the PR's objective of adding iOS build support, as UGUI is crucial for developing cross-platform user interfaces.Some implications to consider:
- This change may enable more sophisticated UI capabilities in VRChat worlds.
- Developers using this package may now have access to additional UI tools and components.
- The stable version (1.0.0) suggests a reliable implementation.
Consider documenting this new dependency in the package's README or documentation to inform developers about the expanded UI capabilities now available in VRChat worlds.
ProjectSettings/ProjectSettings.asset (1)
Line range hint
827-839: Review scripting define symbols for iOSAdding
UDONSHARP;UDON;VRC_SDK_VRCSDK3to thescriptingDefineSymbolsfor iPhone includes VRChat-related code in iOS builds. Since VRChat doesn't officially support iOS, please confirm that this is intentional and that the codebase handles iOS-specific limitations appropriately.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
- .github/workflows/build-listing.yml (1 hunks)
- .github/workflows/push.yml (1 hunks)
- .github/workflows/release.yml (1 hunks)
- .tool-versions (1 hunks)
- .vscode/extensions.json (1 hunks)
- .vscode/settings.json (4 hunks)
- Assets/XR/XRGeneralSettings.asset (2 hunks)
- Packages/black.kit.toybox/Examples/Materials/Floor.mat (2 hunks)
- Packages/black.kit.toybox/Examples/Prefabs/CommonFloor.prefab (1 hunks)
- Packages/black.kit.toybox/Examples/Scenes/GlobalScroll.unity (1 hunks)
- Packages/black.kit.toybox/Examples/Scenes/TickingDown.unity (1 hunks)
- Packages/black.kit.toybox/package.json (2 hunks)
- Packages/packages-lock.json (1 hunks)
- Packages/vpm-manifest.json (1 hunks)
- ProjectSettings/ProjectSettings.asset (8 hunks)
- README.md (1 hunks)
- global.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .tool-versions
- global.json
🧰 Additional context used
🔇 Additional comments (29)
.vscode/extensions.json (1)
6-7: Approved: New VSCode extensions align with PR objectivesThe addition of these two extensions enhances the development experience and aligns with the PR objectives:
- "mariomatheu.syntax-project-pbxproj" supports the new iOS build functionality by providing syntax highlighting for Xcode project files.
- "mrorz.language-gettext" improves the development experience for potential internationalization efforts.
These changes contribute to the mentioned "Improvements to the Visual Studio Code configuration" in the PR objectives.
Packages/vpm-manifest.json (3)
28-30: New locked dependency added correctly.The new locked dependency
com.vrchat.baseversion 3.7.1 has been added, which corresponds to the dependency required bycom.vrchat.worlds. This addition helps maintain version consistency across the project.To ensure this new dependency doesn't introduce any unexpected changes, please:
- Review the documentation of
com.vrchat.baseto understand its purpose and any potential impact on your project.- Check if any existing scripts or components in your project need to be updated to work with this new base package.
#!/bin/bash # Description: Search for potential usage of com.vrchat.base components. echo "Searching for potential usage of com.vrchat.base components:" rg --type csharp "VRCBase" -n
18-19: Locked versions updated correctly.The locked versions for
dev.onevr.vrworldtoolkitandcom.vrchat.worldshave been updated to match the changes in thedependenciessection. Additionally,com.vrchat.worldsnow has a dependency oncom.vrchat.baseversion 3.7.1.Please ensure that the new dependency
com.vrchat.baseis properly resolved and doesn't conflict with any existing dependencies. You can verify this by:
- Checking if
com.vrchat.baseis available in your package repository.- Verifying that no other packages depend on a different version of
com.vrchat.base.#!/bin/bash # Description: Search for usage of com.vrchat.base to identify potential conflicts. echo "Searching for usage of com.vrchat.base:" rg --type json "com.vrchat.base" -nAlso applies to: 22-23, 25-25
6-7: Dependency versions updated successfully.The versions for
dev.onevr.vrworldtoolkitandcom.vrchat.worldshave been updated. This aligns with the PR objective of updating dependencies.Please ensure that these updated versions are compatible with your project and don't introduce any breaking changes. You can verify this by:
- Checking the changelogs or release notes for these packages.
- Running your test suite to catch any potential issues.
- Manually testing the affected functionality in your VRChat world.
Also applies to: 9-10
Packages/black.kit.toybox/package.json (1)
23-23: Dependency update approved. Verify compatibility with the new version.The update of com.vrchat.worlds from ^3.6.1 to ^3.7.1 aligns with the PR objective of "Updates to dependencies".
Please ensure that the project is compatible with the new version of com.vrchat.worlds. Check for any breaking changes or important updates in the 3.7.x version that might affect your project. You can run the following script to search for usage of VRChat Worlds SDK components in your project:
Review the results to identify any potential compatibility issues with the updated SDK version.
README.md (1)
21-21: LGTM: New feature addition aligns with PR objectives.The addition of "Architecture free! (Windows, Android / Quest, iOS)" as the first item in the Features list accurately reflects the PR's focus on enhancing build support for iOS. The emoji choice (📱) is appropriate and consistent with the style of other feature descriptions.
.github/workflows/push.yml (2)
1-8: LGTM: Workflow trigger and permissions are well-configured.The workflow trigger is correctly set up to run on push events for all branches except 'main', which is suitable for a CI workflow. The read-only permissions for contents align with the principle of least privilege, enhancing security.
31-38: LGTM: Automation repository checkout and global.json deployment are well-implemented.The steps for checking out the automation repository and deploying the global.json file are correctly implemented. The use of environment variables and workspace paths is consistent.
Could you provide more context on the purpose and contents of the global.json file? This information would help in understanding its role in the CI process. To verify its contents, you can run the following command:
✅ Verification successful
** Global.json deployment verified successfully.**
The
global.jsonfile contains the correct SDK version and rollForward settings required for the CI process. The deployment step accurately copies the file to the specified path, ensuring the CI environment is properly configured.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Display the contents of the global.json file cat global.jsonLength of output: 95
Packages/black.kit.toybox/Examples/Materials/Floor.mat (4)
14-15: LGTM: Geometric Specular AA enabledThe addition of "_ENABLE_GEOMETRIC_SPECULAR_AA" to the valid keywords enables Geometric Specular Anti-Aliasing for this material. This is a good optimization that can reduce aliasing artifacts in specular reflections, especially on surfaces with high geometric detail.
65-65: Clarify the purpose of the _Bicubic propertyA new "_Bicubic" property has been added with a value of 0. Could you please provide more context on the intended use of this property? Typically, this would relate to texture sampling, but it's currently disabled. Is this a placeholder for future use, or should it be enabled for this material?
71-71: LGTM: Geometric Specular AA configurationThe new properties for Geometric Specular Anti-Aliasing have been added with appropriate default values:
- _EnableGeometricSpecularAA: 1 (enabled)
- _SpecularAAScreenSpaceVariance: 0.1
- _SpecularAAThreshold: 0.2
These settings will help reduce aliasing in specular reflections, improving the visual quality of the material. The values chosen are reasonable defaults that should work well in most scenarios.
Also applies to: 81-82
75-75: Clarify the _LightmapType property settingA new "_LightmapType" property has been added with a value of 0. This typically indicates a non-directional lightmap. Could you please confirm if this is the intended setting for this floor material? Depending on the lighting requirements of your scene, you might want to consider using directional lightmaps (usually value 1) for improved lighting detail.
Assets/XR/XRGeneralSettings.asset (1)
29-29: LGTM: iPhone Settings updated with loader manager instanceThe
m_LoaderManagerInstancefor iPhone Settings has been correctly updated to reference the newly added iPhone Providers section. This change is consistent with the PR objective of adding iOS build support..github/workflows/release.yml (4)
64-64: Correct file extension syntax inzipFilevariable.The addition of the period before the file extension in the
zipFilevariable assignment is correct. This ensures proper file naming for the zip archive.
67-67: Correct YAML syntax forworking-directory.The
working-directorykey has been correctly moved to its own line, at the same level as therunkey. This is the proper YAML syntax for GitHub Actions workflows. This change ensures that the zip command will be executed in the correct directory, preventing potential issues with file paths during the release process.
69-69: Improve specificity of.metafile search.The addition of the
-type fflag to thefindcommand is a good improvement. This flag ensures that only regular files (not directories or other file types) with the.metaextension are included in the search results. This increased specificity helps prevent potential issues that could arise from including unexpected items in the meta file list.
Line range hint
1-105: Overall improvement to the release workflow robustness.The changes made to this workflow file collectively enhance its reliability and correctness. Key improvements include:
- Proper file extension syntax for the zip file.
- Correct YAML structure for the
working-directoryspecification.- More precise
.metafile search.- Safer handling of the
unityPackagevariable in the zip command.These modifications reduce the likelihood of errors during the release process and improve the overall quality of the workflow.
.vscode/settings.json (3)
5-6: Improved file nesting configurationThe new file nesting patterns enhance the organization of files in the VS Code explorer:
"*": "${capture}.meta, ${capture}.md5, ${capture}-lock"groups related meta and lock files."*.db": "${capture}.db.st"nests database-related files."*.png": "${capture}@*.png"groups different resolutions of PNG images."*.xml": "${capture}.dll"associates XML files with their corresponding DLLs.These changes improve the project's file structure visibility and are consistent with Unity project conventions.
Also applies to: 9-9, 11-11
98-133: Expanded Unity YAML schema supportThe addition of numerous Unity-specific YAML tags significantly enhances VS Code's ability to validate and provide IntelliSense for Unity YAML files. This comprehensive list covers a wide range of Unity's internal structures, which should improve the development experience when working with Unity-generated YAML files.
These additions are beneficial, but it's important to ensure they align with the Unity version used in the project.
To verify the compatibility of these YAML tags with your Unity version, please run the following script:
#!/bin/bash # Check Unity version and compare with the added YAML tags unity_version=$(grep -oP '(?<=m_EditorVersion: )\S+' ProjectSettings/ProjectVersion.txt) echo "Unity Version: $unity_version" echo "Number of Unity YAML tags: $(grep -c 'tag:unity3d.com' .vscode/settings.json)" echo "Highest Unity YAML tag number: $(grep 'tag:unity3d.com' .vscode/settings.json | grep -oP '(?<=:)\d+' | sort -rn | head -n1)"This script will output the Unity version used in the project and some statistics about the YAML tags. Please ensure that the tags are appropriate for your Unity version.
25-25: Enhanced file type associationsThe updates to file associations improve VS Code's handling of various file types:
- New associations for
*.exe.config,*.nuget.cache,*.uxml,.buginfo,CodeResources, andProjectCacheenhance syntax highlighting and editor features for these files.- The
*.infoassociation has been reinstated as JSON, which is appropriate for most .info files in Unity projects.One change to note:
- The association for
*.tlhfiles has been changed from C to C++. While this is likely correct, it's worth verifying that this change doesn't negatively impact any existing C-specific tooling or IntelliSense features for these files.To ensure the
.tlhfile association change doesn't cause any issues, please run the following script:Also applies to: 27-27, 32-32, 41-41, 45-46, 49-49, 53-53
✅ Verification successful
Verified file association changes
No
.tlhfiles were found in the repository. The change in file association from C to C++ for*.tlhfiles will not impact the current project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any .tlh files and their content to verify C++ compatibility fd -e tlh -x echo "File: {}" \; -x head -n 10 {} \;Length of output: 470
Packages/black.kit.toybox/Examples/Prefabs/CommonFloor.prefab (1)
266-266: Verify the necessity of theNavigationAreaspropertyThe
NavigationAreasproperty has been added as an empty array to theVRCWorldMonoBehaviour. If this property is not being actively used or populated, it might be unnecessary to include it. Please confirm whether this addition is intentional and required for future configurations.Run the following script to check for references to
NavigationAreasin the codebase:✅ Verification successful
NavigationAreasProperty is UnusedThe
NavigationAreasproperty does not have any references or usages in the codebase across YAML, C#, and JSON files. It appears to be unnecessary and can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for references to `NavigationAreas` in YAML and C# files. # Test: Search for usage of `NavigationAreas`. Expect: Instances where `NavigationAreas` is accessed or modified. rg --type yaml --type cs 'NavigationAreas'Length of output: 42
Script:
#!/bin/bash # Description: Enhanced search for references to `NavigationAreas` across additional file types and with case-insensitive matching. # Search for `NavigationAreas` in YAML, C#, and JSON files, case-insensitively. rg --ignore-case 'NavigationAreas' --type yaml --type cs --type jsonLength of output: 68
ProjectSettings/ProjectSettings.asset (6)
174-174: Ensure compatibility with Android SDK 33Updating
AndroidTargetSdkVersionto33targets the latest Android SDK. Please verify that all plugins and dependencies are compatible with SDK 33 to prevent any build or runtime issues.
262-262: Confirm the Android target architecturesChanging
AndroidTargetArchitecturesto2modifies the build to target specific CPU architectures. Ensure that this value corresponds to the intended architecture (e.g., ARM64, x86) and that the project's plugins and libraries support it.
510-510: Graphics Jobs enabled for iOSEnabling
m_GraphicsJobsfor iOS can improve performance but may introduce rendering issues on certain devices. It's important to thoroughly test the application on various iOS devices to ensure stability and visual correctness.
518-518: Graphics Jobs enabled for AndroidSimilarly, enabling
m_GraphicsJobsfor Android could benefit performance but might cause instability on some hardware. Please test across a range of Android devices to confirm reliability.
532-532: Automatic graphics API selection for iOSSetting
m_Automaticto1enables Unity to automatically choose the graphics API for iOS. If the project relies on specific graphics APIs, ensure this change doesn't conflict with those requirements.
874-874: API compatibility level set to .NET Standard 2.0 for iOSSetting
apiCompatibilityLevelPerPlatformfor iPhone to3configures the API compatibility level to .NET Standard 2.0. This is a standard choice and should be compatible with most libraries.Packages/black.kit.toybox/Examples/Scenes/TickingDown.unity (1)
4947-4951: Verify Material ReferenceThe material is assigned via
objectReferencewith GUID35630e38b4e5ffb40b44ec5348b4333f. Please ensure that this material exists in the project and is correctly referenced to prevent runtime errors.Run the following script to verify the existence of the material:
✅ Verification successful
Material Reference Verified
The material with GUID
35630e38b4e5ffb40b44ec5348b4333fexists in the project and is correctly referenced in the specified files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the material with GUID '35630e38b4e5ffb40b44ec5348b4333f' exists in the project. # Test: Search for files containing the material GUID. Expect: At least one matching file. rg "guid: 35630e38b4e5ffb40b44ec5348b4333f"Length of output: 7205
Packages/black.kit.toybox/Examples/Scenes/GlobalScroll.unity (1)
4788-4788: Confirm the change tom_AnchoredPosition.yvalueThe
m_AnchoredPosition.yproperty has been updated to557.019. Please verify that this adjustment aligns with the intended positioning of the UI element within the scene, as it may affect the layout on different screen resolutions or aspect ratios.
| AndroidPreferredInstallLocation: 0 | ||
| aotOptions: | ||
| stripEngineCode: 1 | ||
| iPhoneStrippingLevel: 0 | ||
| iPhoneScriptCallOptimization: 0 | ||
| ForceInternetPermission: 0 | ||
| ForceSDCardPermission: 0 | ||
| ForceSDCardPermission: 1 |
There was a problem hiding this comment.
Assess the necessity of forcing SD card permission
Setting ForceSDCardPermission to 1 will request SD card access from users. If the application doesn't require external storage access, consider disabling this to enhance user privacy and reduce unnecessary permission requests.
| - m_BuildTarget: iPhone | ||
| m_Icons: | ||
| - m_Textures: [] | ||
| m_Width: 180 | ||
| m_Height: 180 | ||
| m_Kind: 0 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 120 | ||
| m_Height: 120 | ||
| m_Kind: 0 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 167 | ||
| m_Height: 167 | ||
| m_Kind: 0 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 152 | ||
| m_Height: 152 | ||
| m_Kind: 0 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 76 | ||
| m_Height: 76 | ||
| m_Kind: 0 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 120 | ||
| m_Height: 120 | ||
| m_Kind: 3 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 80 | ||
| m_Height: 80 | ||
| m_Kind: 3 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 80 | ||
| m_Height: 80 | ||
| m_Kind: 3 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 40 | ||
| m_Height: 40 | ||
| m_Kind: 3 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 87 | ||
| m_Height: 87 | ||
| m_Kind: 1 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 58 | ||
| m_Height: 58 | ||
| m_Kind: 1 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 29 | ||
| m_Height: 29 | ||
| m_Kind: 1 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 58 | ||
| m_Height: 58 | ||
| m_Kind: 1 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 29 | ||
| m_Height: 29 | ||
| m_Kind: 1 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 60 | ||
| m_Height: 60 | ||
| m_Kind: 2 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 40 | ||
| m_Height: 40 | ||
| m_Kind: 2 | ||
| m_SubKind: iPhone | ||
| - m_Textures: [] | ||
| m_Width: 40 | ||
| m_Height: 40 | ||
| m_Kind: 2 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 20 | ||
| m_Height: 20 | ||
| m_Kind: 2 | ||
| m_SubKind: iPad | ||
| - m_Textures: [] | ||
| m_Width: 1024 | ||
| m_Height: 1024 | ||
| m_Kind: 4 | ||
| m_SubKind: App Store |
There was a problem hiding this comment.
Verify iOS app icon assignments
New icon entries for iPhone have been added without assigned textures. To comply with App Store requirements, please ensure all required icon slots are filled with appropriate images to prevent rejection during the submission process.
The main focus of this update is to improve build configuration and CI/CD scripts for iOS, but the only update to the product is a dependency update.
Small features
0.8.1CI/CD
Other updates
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores