inkle / ink-unity-integration

Unity integration for the open source ink narrative scripting language.
http://www.inklestudios.com/ink
Other
582 stars 101 forks source link

InkLibraryAsset is overwritten with an empty one when opening a project for the first time #56

Closed gboulard closed 6 years ago

gboulard commented 6 years ago

When we retrieve the unity project on a new machine (via control source) , the InkLibrary.asset is overwritten with an empty one.

this is due to the fact the code check the existence of a key in EditorPrefs (see FindOrCreateSingletonScriptableObjectOfType)

is there any particular reason to use an EditorPrefs Key ?

tomkail commented 6 years ago

We use the editorprefs key because finding the InkLibrary asset via AssetDatabase.FindAssets is quite expensive, and storing the asset reference this way saves calling it each time unity recompiles. What's really happening here, I think, is that it fails to find the editorpref (as expected for a new machine), and then tries to find the asset via AssetDatabase.FindAssets but fails because it's not yet imported it. Since it thinks that there's no library, it creates a new one. The kicker is that the system should resolve this on recompile when it finds two files by deleting the newest one - but since you must have the asset in the default path, creating a new asset overwrites the old one. We can fix this specific case by attempting to import at the default path but that's not really a robust fix. The only fix I can think of is to search the file directory for the file rather than the AssetDatabase, but that's really horrible since there's no way to get the asset type without actually reading the file. So I'm stumped! Anyone in the community have a fix for this in mind?

On Fri, Mar 23, 2018 at 10:19 AM, gboulard notifications@github.com wrote:

When we retrieve the unity project on a new machine (via control source) , the InkLibrary.asset is overwritten with an empty one.

this is due to the fact the code check the existence of a key in EditorPrefs (see FindOrCreateSingletonScriptableObjectOfType)

is there any particular reason to use an EditorPrefs Key ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/inkle/ink-unity-integration/issues/56, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMd0Djw4CrNDHMaYFgrCDeSfr9cIIrqks5thMwZgaJpZM4S4ebh .

KumoKairo commented 6 years ago

Doesn't storing it in the Editor Prefs make the Ink Library path to be per-machine rather than per-project? What if I have three Ink projects? Woudn't the Editor Prefs value point only to the last one opened?

saves calling it each time unity recompiles.

If it's stored in ScriptableObject, it survives recompilation (as well as Play/Stop routine, it's called "Assembly Reload") and can be set to a singleton static reference right after that, so no need to call FindAssets or anything like that.

Anyone in the community have a fix for this in mind?

There's a common fix for this that enabled per-project preferences, think of it as a per-project Editor Prefs line. We can create a text file inside of the root folder (one level higher than Assets, the one where .sln and .csproj files are located) and store any cached paths there. The file will register itself in the version control system and won't suffer from one-cache-line-per-machine issues.

tomkail commented 6 years ago

We use a playerprefs key based on the project name to get around that one.

As you say, it should be fine after that first import.

The file you suggest sounds good to me, although it’s a shame to need an extra file! For extra credit, I’d be very interested in ways of saving and loading a scriptable object into the project setting folder that contains the settings for input etc.

KumoKairo commented 6 years ago

it’s a shame to need an extra file!

Technically EditorPrefs does that as well, it's just a hidden file

I’d be very interested in ways of saving and loading a scriptable object into the project setting folder that contains the settings for input etc.

I'd look into a few ways of addressing it, will write back by the end of the next week (May 13) with the results

KumoKairo commented 6 years ago

Okay, I confirm that ScriptableObject's OnEnable / OnDisable method is working okay, without the need of accessing any separate files or EditorPrefs. The technique just uses those Unity Messages on ScriptableObjects to hard-set the static _Instance field for the Library. Methods are called in needed order, without any "missed" calls like it happens in some cases, so there are no calls to those FastFindAndEnforceSingletonScriptableObjectOfType methods.

I have also noted the same EditorPrefs technique with the InkSettings file too, so it can also be replaced with a simpler one.

I would also suggest storing Last Compile Time directly inside of the InkLibrary file so everything remains project-based instead of machine-based.

One question remains though, about the UX of the new Ink Library file creation. If I delete one, the code of InkLibrary-related stuff doesn't try to create a new one until I reopen the project. Is there a reason behind it? Shoudln't it be creating a new default file once the old one is deleted?

See https://github.com/KumoKairo/ink-unity-integration/commit/34e93f94602924a25a5b80e28adaebae23bab45d for the changes

tomkail commented 6 years ago

Oh nice! I always forget that you get enable on SOs. I’d want to test it for a bit to make sure they’re always ready when assets are imported, but it seems like a good idea. Hmm the ink library is supposed to create itself if it doesn’t exist whenever it’s needed - is that not working?

On Mon, 7 May 2018 at 11:47, Kirill notifications@github.com wrote:

Okay, I confirm that ScriptableObject's OnEnable / OnDisable method is working okay, without the need of accessing any separate files or EditorPrefs. The technique just uses those Unity Messages on ScriptableObjects to hard-set the static _Instance for the Library. Methods are called in needed order, without any "missed" calls like it happens in some cases, so there are no calls to those FastFindAndEnforceSingletonScriptableObjectOfType methods.

I have also noted the same EditorPrefs technique with the InkSettings file too, so it can also be replaced with a simpler one.

I would also suggest storing Last Compile Time directly inside of the InkLibrary file so everything remains project-based instead of machine-based.

One question remains though, about the UX of the new Ink Library file creation. If I delete one, the code of InkLibrary-related stuff doesn't try to create a new one until I reopen the project. Is there a reason behind it? Shoudln't it be creating a new default file once the old one is deleted?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/inkle/ink-unity-integration/issues/56#issuecomment-387028348, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMd0C2bGojwZhTrXAj-3hs6N7nzdAtLks5twCY9gaJpZM4S4ebh .

KumoKairo commented 6 years ago

Hmm the ink library is supposed to create itself if it doesn’t exist whenever it’s needed - is that not working?

The code that's in the Compiler's Update method only checks if a Library exists or tries to find it, without creating. It only tries to create one at Unity startup. So I don't think it's the definition of "not working", it's looks like expected behavior

tomkail commented 6 years ago

I've got this in our project and I'm going to let it settle for a bit. I'm still wary of if it'll fix the original bug (although it's clearly better code if it works) because of how Unity sometimes staggers importing assets but it looks hopeful! Thanks for your work!

KumoKairo commented 6 years ago

I've found a small issue with the static link check (should check for _link != this), other than that it's working OK in our project. How about spreading the "store everything in SO" tactics further, to completely eliminate EditorPrefs?

tomkail commented 6 years ago

I've made some minor changes too :) Feel free to try, although bear in mind that we use prefs for the ink meta information so that it doesn't get churned in source control.

KumoKairo commented 6 years ago

Can you specify which meta-info is required to be local?

tomkail commented 6 years ago

There’s a class called inkmetainfo or something - it stores relationships between files.

On Thu, 17 May 2018 at 17:31, Kirill notifications@github.com wrote:

Can you specify which meta-info is required to be local?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/inkle/ink-unity-integration/issues/56#issuecomment-389928683, or mute the thread https://github.com/notifications/unsubscribe-auth/ABMd0OPxRZNmkX7mBnBMsgi26BD2uZZxks5tzaXngaJpZM4S4ebh .

tomkail commented 6 years ago

I've pushed your changes (and a few of my own). I'm going to close this issue since it's been fixed, but if you manage to find a nicer way to store InkMetaFile data than playerprefs I'd love to get it in!