Fix relative paths when called from package cache and local packages#180
Fix relative paths when called from package cache and local packages#180nowsprinting wants to merge 7 commits intomasterfrom
Conversation
- Add PathHelper.ResolveUnityPath() to resolve relative paths to Unity format - Remove duplicate GetAbsolutePath() from LoadAssetAttribute and SceneManagerHelper - Support Embedded packages by detecting package.json and converting paths Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
nowsprinting
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors path resolution logic by consolidating duplicate GetAbsolutePath() methods into a single PathHelper.ResolveUnityPath() method and adds support for embedded packages. The refactoring removes code duplication from SceneManagerHelper and LoadAssetAttribute classes.
Changes:
- Consolidated three duplicate
GetAbsolutePath()implementations into a singlePathHelper.ResolveUnityPath()method - Added embedded package support with automatic detection of
package.jsonand path conversion - Moved conditional compilation directive for
Cysharp.Threading.Tasksto properly scope the using statement
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/RuntimeInternals/SceneManagerHelperTest.cs | Removed duplicate GetAbsolutePath test, updated copyright, improved conditional compilation for Unity 2022+ |
| Tests/RuntimeInternals/PathHelperTest.cs | Added ResolveUnityPath test cases covering various path scenarios, updated copyright |
| Tests/Runtime/Attributes/LoadAssetAttributeTest.cs | Removed duplicate GetAbsolutePath test, updated copyright |
| RuntimeInternals/SceneManagerHelper.cs | Removed duplicate GetAbsolutePath method, updated to use PathHelper.ResolveUnityPath, updated copyright |
| RuntimeInternals/PathHelper.cs | Added ResolveUnityPath method with embedded package support and helper methods, updated copyright |
| Runtime/Attributes/LoadAssetAttribute.cs | Removed duplicate GetAbsolutePath method, updated to use PathHelper.ResolveUnityPath, updated copyright |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| catch | ||
| { |
There was a problem hiding this comment.
The catch block on line 274 silently swallows all exceptions without logging. While the method returns null to indicate failure, this makes debugging difficult when issues occur during file reading or JSON parsing. Consider logging the exception using Debug.LogWarning or Debug.LogError to help diagnose problems in production.
| catch | |
| { | |
| catch (Exception ex) | |
| { | |
| Debug.LogWarning($"Failed to read or parse package.json at '{packageJsonPath}': {ex}"); |
| // Simple JSON parsing to extract "name": "..." | ||
| var nameIndex = json.IndexOf("\"name\"", StringComparison.Ordinal); | ||
| if (nameIndex < 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var colonIndex = json.IndexOf(":", nameIndex, StringComparison.Ordinal); | ||
| if (colonIndex < 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var openQuoteIndex = json.IndexOf("\"", colonIndex, StringComparison.Ordinal); | ||
| if (openQuoteIndex < 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var closeQuoteIndex = json.IndexOf("\"", openQuoteIndex + 1, StringComparison.Ordinal); | ||
| if (closeQuoteIndex < 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| return json.Substring(openQuoteIndex + 1, closeQuoteIndex - openQuoteIndex - 1); | ||
| } | ||
| catch | ||
| { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The simple string-based JSON parsing in GetPackageNameFromJson is fragile and may fail with valid JSON that has whitespace variations, comments, or different formatting. For example, it won't handle cases where there are spaces after the colon (: "name"), or where the "name" field is not the first field with quotes. Consider using a proper JSON parser (like Unity's JsonUtility or a lightweight JSON library) for more robust parsing, or at minimum, add logic to skip whitespace after the colon.
| // Simple JSON parsing to extract "name": "..." | |
| var nameIndex = json.IndexOf("\"name\"", StringComparison.Ordinal); | |
| if (nameIndex < 0) | |
| { | |
| return null; | |
| } | |
| var colonIndex = json.IndexOf(":", nameIndex, StringComparison.Ordinal); | |
| if (colonIndex < 0) | |
| { | |
| return null; | |
| } | |
| var openQuoteIndex = json.IndexOf("\"", colonIndex, StringComparison.Ordinal); | |
| if (openQuoteIndex < 0) | |
| { | |
| return null; | |
| } | |
| var closeQuoteIndex = json.IndexOf("\"", openQuoteIndex + 1, StringComparison.Ordinal); | |
| if (closeQuoteIndex < 0) | |
| { | |
| return null; | |
| } | |
| return json.Substring(openQuoteIndex + 1, closeQuoteIndex - openQuoteIndex - 1); | |
| } | |
| catch | |
| { | |
| return null; | |
| } | |
| } | |
| var packageInfo = JsonUtility.FromJson<PackageJson>(json); | |
| if (packageInfo == null || string.IsNullOrEmpty(packageInfo.name)) | |
| { | |
| return null; | |
| } | |
| return packageInfo.name; | |
| } | |
| catch | |
| { | |
| return null; | |
| } | |
| } | |
| [Serializable] | |
| private class PackageJson | |
| { | |
| public string name; | |
| } |
RuntimeInternals/PathHelper.cs
Outdated
| // Check if the first directory has package.json | ||
| var firstDirectory = segments[0]; | ||
| var packageJsonPath = Path.Combine(firstDirectory, "package.json"); | ||
|
|
There was a problem hiding this comment.
The ConvertToUnityPath method attempts to check if a package.json file exists using a relative path segment without combining it with the project root directory. On line 225, packageJsonPath is constructed using only firstDirectory (a path segment like "MyPackage"), which is relative. When File.Exists(packageJsonPath) is called on line 227, it will check relative to the current working directory instead of the project root. This method needs to accept the project root as a parameter and construct the full path as Path.Combine(projectRoot, firstDirectory, "package.json") to work correctly.
| // Check if the first directory has package.json | |
| var firstDirectory = segments[0]; | |
| var packageJsonPath = Path.Combine(firstDirectory, "package.json"); | |
| // Check if the first directory has package.json in the project root | |
| var firstDirectory = segments[0]; | |
| var projectRoot = FindProjectRoot(Application.dataPath); | |
| if (string.IsNullOrEmpty(projectRoot)) | |
| { | |
| return null; | |
| } | |
| var packageJsonPath = Path.Combine(projectRoot, firstDirectory, "package.json"); |
| // If neither found, it might be an Embedded package | ||
| // Find project root and convert to Packages/{packageName}/ format | ||
| var projectRoot = FindProjectRoot(absolutePath); | ||
| if (projectRoot != null) | ||
| { | ||
| var relativeFromRoot = absolutePath.Substring(projectRoot.Length).TrimStart(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); | ||
| var unityPath = ConvertToUnityPath(relativeFromRoot); | ||
| if (unityPath != null) | ||
| { | ||
| return unityPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new embedded package functionality introduced in ResolveUnityPath (lines 173-184) lacks test coverage. The existing test cases only cover standard Assets/ and Packages/ paths, but don't test the embedded package scenario where neither "Assets" nor "Packages" is found in the absolute path. Consider adding test cases for embedded package paths to verify this functionality works correctly.
This comment has been minimized.
This comment has been minimized.
nowsprinting
changed the title
This comment has been minimized.
This comment has been minimized.
Update ConvertToUnityPath() to search for package.json at any depth (from deep to shallow) instead of only checking the first directory. Use absolute paths for File.Exists() checks to resolve paths correctly for local packages placed outside Packages/ directory (e.g., LocalPackages/). This fixes path resolution failures when using relative paths from local packages that are nested deeper in the directory structure. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Change directory detection to search for paths surrounded by path separators (e.g., /Assets/, /Packages/) instead of just the directory name. This prevents false matches with directory names that contain these strings (e.g., "LocalPackages" contains "Packages"). Also fix test case relative path from ../../ to ../ to match actual file structure in LocalPackages/TestsFromLocalPackage/. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Use real path (relative from project root) for Resources path mapping between build time and runtime, instead of relying on CallerFilePath which behaves differently in editor vs standalone player. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Skip path resolution and validation for LoadScene in standalone player - Player only needs scene name, not full path - Remove debug logs from LoadAssetAttribute and TemporaryCopyAssetsForPlayer Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Code Metrics Report
Details | | master (d8328f9) | #180 (0c79473) | +/- |
|---------------------|------------------|----------------|-------|
- | Coverage | 79.4% | 73.4% | -6.1% |
| Files | 44 | 44 | 0 |
| Lines | 1581 | 1690 | +109 |
- | Covered | 1256 | 1241 | -15 |
- | Code to Test Ratio | 1:1.0 | 1:0.9 | -0.1 |
| Code | 2917 | 3079 | +162 |
+ | Test | 3077 | 3078 | +1 |
+ | Test Execution Time | 6m4s | 5m30s | -34s |Code coverage of files in pull request scope (71.8% → 44.4%)
Reported by octocov |
nowsprinting
changed the title
Problem
When using local packages (referenced via
file:in manifest) or packages from the cache (Library/PackageCache/),Path.GetFullPathreturns the actual filesystem path instead of the Unity virtual path.For example, with a local package:
Packages/com.nowsprinting.test-helper/Tests/Scenes/Scene.unity/Users/.../project-root/test-helper/Tests/Scenes/Scene.unityThe previous implementation searched for
Assets/orPackages/in the resolved absolute path, but neither exists in the actual filesystem path.Fixes