Skip to content

use cross-platform paths for migration#26

Merged
PiMaker merged 1 commit intoPiMaker:mainfrom
galister:main
Dec 26, 2023
Merged

use cross-platform paths for migration#26
PiMaker merged 1 commit intoPiMaker:mainfrom
galister:main

Conversation

@galister
Copy link
Contributor

@galister galister commented Dec 9, 2023

Issue:

On platforms other than Windows, the migration process creates LTCGI.cginc and gizmo files outside of the Assets folder.

Dependent shaders will error out due to the include path not pointing to a valid file.

drwxr-xr-x 10 op op  4096 Dec  9 10:40  ./
drwxr-xr-x 13 op op  4096 Dec  8 19:57  ../
drwxr-xr-x 19 op op  4096 Dec  8 23:11  Assets/
-rw-r--r--  1 op op 11809 Dec  8 20:30 'Assets\Gizmos\LTCGI_Screen_Gizmo.png'
-rw-r--r--  1 op op    56 Dec  8 20:30 'Assets\_pi_\_LTCGI\Shaders\LTCGI.cginc'
-rw-r--r--  1 op op   403 Dec  8 20:03  Avatars22.sln
drwxr-xr-x 15 op op  4096 Dec  9 10:41  Library/
drwxr-xr-x  2 op op  4096 Dec  9 10:41  Logs/
drwxr-xr-x  7 op op  4096 Dec  8 22:53  Packages/
drwxr-xr-x  3 op op  4096 Dec  9 10:41  ProjectSettings/
drwxr-xr-x  4 op op  4096 Dec  9 10:45  Temp/
drwxr-xr-x  3 op op  4096 Dec  9 10:38  UserSettings/

Proposed solution

  1. Use Path.Combine instead of hardcoded path separators
  2. Use Directory.CreateDirectory to create the folder structure recursively. (Using AssetDatabase is not necessary, as we're not injecting any metadata.)

Tested

On Linux, but the behavior is expected to be the same on all platforms.

@PiMaker
Copy link
Owner

PiMaker commented Dec 26, 2023

Had to double-check that Directory.CreateDirectory creates the full hierarchy if multiple folders are missing, but this looks to be correct.

Thank you for the contribution! I'll make sure to give it a quick test on Windows too and ship it with the next release (though I don't know when I'll have time for that yet).

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