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

Add load/save functionality to the FileSystem extension #884

Merged
merged 12 commits into from
Jan 24, 2019

Conversation

Wend1go
Copy link
Contributor

@Wend1go Wend1go commented Jan 16, 2019

  • Add 'save text to file' function
  • Add 'load text from file' function
  • Add 'delete file' function
  • Add convenience functions to automatically convert JSON encoded file content into a structure
  • Update the wiki article

The save / load functions can be used with Json strings just like the Storage extension does.

@4ian
Copy link
Owner

4ian commented Jan 17, 2019

Looks like a great start 😊👍

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 18, 2019

I am contemplating about how to code the loadJSONFileFromFileSystem (as an equivalent to loadJSONFileFromStorage).
In line 48 of https://github.com/4ian/GDevelop/blob/master/GDJS/Runtime/events-tools/storagetools.js#L48 the JSON string is saved via gdjs.evtTools.storage.loadedFiles.put(filename, JSON.parse(rawStr));
Since I'm basically trying to do do the same thing (loading the JSON encoded content of the file into memory for further access), would it be ok to access gdjs.evtTools.storage.loadedFiles from within this extension even though it has nothing to do with the web storage?

Rename "text" to "string" in javascript functions
Make "err" a separate argument in console output
@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 19, 2019

I used the asynchronous variant of the unlink (delete) function to not block the game.
If the game designer needs to be sure that the file was successfully deleted before doing other stuff, it can be checked with the PathExists function.

@4ian
Copy link
Owner

4ian commented Jan 19, 2019

I used the asynchronous variant of the unlink (delete) function to not block the game.

Yep, that's a good idea :)

Since I'm basically trying to do do the same thing (loading the JSON encoded content of the file into memory for further access), would it be ok to access gdjs.evtTools.storage.loadedFiles from within this extension even though it has nothing to do with the web storage?

Let's avoid accessing gdjs.evtTools.storage, because it's a quite specialized extension and you can even consider that it's not guaranteed that this extension exists (even if it's builtin, at some point it could become an extension like any other).

But I'm not sure why you're dealing with JSON here? As this API is low level, and as you've added gdjs.fileSystem.saveStringToFile, I think you should also provide gdjs.fileSystem.loadStringFromFile: it will read the specified path, and store the content inside the given scene variable (storing loaded things in a scene variable is the way to go I think).
Both of these actions don't care at all about whatever format the text is using - they are just writing/reading raw text.

If someone want to load JSON, the user can:

  • load the content of a file (using "loadStringFromFile") inside a temporary variable
  • Use "Convert JSON to a variable" action to convert the content of this temporary variable into another variable.

If the user want to save JSON from a variable:

  • Use "saveStringToFile" that you already made, using Convert variable to JSON expression (i.e: passing ToJSON(MyVariable) as parameter for the content to save in the file).

By the way, what I wrote here should go in a "how to" page on the wiki :) There are already a few explanations about it here: http://wiki.compilgames.net/doku.php/gdevelop5/all-features/network

This being said, maybe it's also good to provide, in addition to previous methods, two functions to save/load directly a variable from/to a file containing JSON (let's call them gdjs.fileSystem.saveVariableToJSONFile and gdjs.fileSystem.loadVariableFromJSONFile). In this case, use:

gdjs.evtTools.network.variableStructureToJSON to convert a variable to a JSON.
gdjs.evtTools.network.jsonToVariableStructure to dump JSON into a variable.

(it's using gdjs.evtTools.network, which looks like private apis, a bit like gdjs.evtTools.storage. The only difference is that I plan to refactor these two functions to put them later into gdjs.Variable. I've added "TODOs" comments to do this - but never took the time to do it. Anyway, you can use them freely - there will always be there, they are builtin in the game engine :) If I move them at some point, I'll change them in the whole codebase).

What do you think? Does that makes sense?

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 19, 2019

Yes that makes sense.
The reason why I wanted to access gdjs.evtTools.storage was because I had lags caused by frequent saving in my Sakawochi game until I discovered the "Load a structured file in memory" action which made the game butter smooth again. So I wanted to replicate this function for the filesystem extension.
But saving a game that frequently is probably a corner case that only few game developers will ever need to do so I guess I can leave it out. If such functionality is indeed needed one can still safe to the storage repeatedly and do a "backup" on filesystem once when the game gets is closed.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 20, 2019

At first I went with the Async file loading but I think it is pretty confusing for non programmers when you load the text in one action and try to access it in the following action of the same event but get an empty string instead of the expected file content.
So I decided to add a synchronous and an asynchronous variant of the functions.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 20, 2019

Does gdjs.evtTools.network.variableStructureToJSON convert the variable and all its children into a JSON string? Since the text file can't be used like the storage dictionary I'd need to write everything into the file at once.

@4ian
Copy link
Owner

4ian commented Jan 20, 2019

At first I went with the Async file loading but I think it is pretty confusing for non programmers when you load the text in one action and try to access it in the following action of the same event but get an empty string instead of the expected file content.

Yes that's a good idea to have a default "sync" action, and have an async version for more advanced users and larger games/files.

Does gdjs.evtTools.network.variableStructureToJSON convert the variable and all its children into a JSON string

Yes, it's recursive. You get a JSON representing the whole variable (and can "decode" back the JSON to a structure variable which will be exactly the same, using the other method gdjs.evtTools.network.jsonToVariableStructure ).

Since the text file can't be used like the storage dictionary I'd need to write everything into the file at once.

Yes, you dump the whole JSON converted from the variable directly into the file without any transformation. JSON is self sufficient by itself :)

@4ian
Copy link
Owner

4ian commented Jan 20, 2019

The code looks pretty good! :) Added a few comments, one about adding support for an error variable. I think it would be useful to make robust games that warn the user if something wrong happened. It's something that I did for the Facebook Instant Games actions, and also a convention that we can use for other asynchronous actions and actions that can fail.

More pleasant formulation of descriptions
@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 21, 2019

I have added the optional variable. But not just for errors but also for success.
This way the variable result can also be used to determine when the asynchronous operation has finished and if it was successful (result='ok') or had an error (result='error'). The error is still printed to the console.
I don't think it is important for the game to know exactly what went wrong though. At least I haven't encountered a single game that gave me a detailed information about what happened when I tried to save a file and it didn't succeed.

Matthias Meike added 2 commits January 21, 2019 19:51
…structure

Add function to save a structure as JSON string into a file
Add sync and async variants of the existing functions
Cleanup variable names and error result
@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 22, 2019

Some things I noticed:

  1. You can manually write a JSON string into a variable and save it to a file via variableStructureToJSON. But when you try to read it from the file and convert it back into a structure it won't work because of the quotation marks.

This structure for example ...
grafik
generates this output in the file

{"ChildVariable": "{
    "fruit": "Apple",
    "size": "Large",
    "color": "Red"
}"}

If you write the ChildVariable as text and convert it into a structure it works. It just seems to have problems when a JSON string with quotation marks is inside some variable.

  1. Somehow the order of Actions in the list got distorted in the last release. The sub folders are no longer at the top but inbetween the actions which looks kind of weird and makes them harder to find:
    grafik
    How do the actions get sorted in the list? It is neither alphabetically nor in the order of definition in the JsExtension.js file.

@4ian
Copy link
Owner

4ian commented Jan 22, 2019

You can manually write a JSON string into a variable and save it to a file via variableStructureToJSON. But when you try to read it from the file and convert it back into a structure it won't work because of the quotation marks.

You're not writing JSON ;) You're writing a text, that happened to look like JSON. Hence the quotation mark.

This is JSON:
{"hello": "world"}

But variable are text or number. If you enter a number, then a number is stored. Otherwise a text is stored. If you write {"hello": "world"}, GDevelop will store in memory the text {"hello": "world"}. When you will convert it to JSON, it will write: "{\"hello\": \"world\"}". See the difference?

And this is a good thing! Otherwise, imagine someone writing as its player name {injection: "hello"}. You would have just injected JSON into the game, potentially breaking it.

The equivalent of JSON in GDevelop are the structure variables.

How do the actions get sorted in the list? It is neither alphabetically nor in the order of definition in the JsExtension.js file.

There is now way to sort them. It should be done in EnumerateInstruction.js in the IDE.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 23, 2019

I think you got me wrong. What I mean is that when I save the structure with quotation marks inside as the content of the text variable into a file and try to convert it back into a structure, I would expect it to escape the quotation marks when saving to file and recreating the original structure with the quotation marks inside the string variable.

Variable (Structure)
    ChildVariable (Text) = My text with "quotation" marks

But instead the function jsonToVariableStructure just creates an empty structure since it cant convert the serialized JSON string back into the original structure.

@4ian
Copy link
Owner

4ian commented Jan 23, 2019

Very strange then... Something is weird.

Can you post here the content of the JSON that is outputted? :)
Can you add a breakpoint or a console log before calling jsonToVariableStructure and paste here the content of the argument that is sent to jsonToVariableStructure?

@4ian
Copy link
Owner

4ian commented Jan 23, 2019

@Wend1go My bad, I've verified the implementation of gdjs.evtTools.network.variableStructureToJSON, and it's not escaping at all the content of strings 😡😕

Totally misunderstood your first message reporting this issue.

Can't believe how I did not do this on the first place... Anyway, let me work and push a fix on master for this.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 23, 2019

The structure:
grafik

This is what gets parsed into the text file:

{"JSONString": "{
    "fruit": "Apple",
    "size": "Large",
    "color": "Red"
}"}

And this is the error of jsonToVariableStructure when trying to turn it into a structure again:

message:
"Unexpected token ↵ in JSON at position 17"
stack:
"SyntaxError: Unexpected token ↵ in JSON at position 17↵    at JSON.parse (<anonymous>)↵    at Object.gdjs.evtTools.network.jsonToVariableStructure (file:///tmp/preview/events-tools/networktools.js:115:18)↵    at Object.gdjs.fileSystem.loadStructureFromJSONFile (file:///tmp/preview/Extensions/FileSystem/filesystemtools.js:279:17)↵    at Object.gdjs.NewSceneCode.eventsList0xb0b08 (file:///tmp/preview/code0.js:51:18)↵    at gdjs.RuntimeScene.gdjs.NewSceneCode.func [as _eventsFunction] (file:///tmp/preview/code0.js:72:19)↵    at gdjs.RuntimeScene.renderAndStep (file:///tmp/preview/runtimescene.js:232:10)↵    at gdjs.SceneStack.step (file:///tmp/preview/scenestack.js:30:22)↵    at file:///tmp/preview/runtimegame.js:354:26↵    at gameLoop (file:///tmp/preview/pixi-renderers/runtimegame-pixi-renderer.js:380:13)"

EDIT:
Ah nice, you found the error. 👍

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 23, 2019

I think this should be ready for a review then.
If you are ok with the use of the result variable, I'd start writing the wiki article.

@4ian
Copy link
Owner

4ian commented Jan 23, 2019

Yup, gdjs.evtTools.network.variableStructureToJSON is outputting totally invalid JSON.
Can you merge master in your branch? I've just pushed a fix to gdjs.evtTools.network.variableStructureToJSON so that it create a valid JSON, whatever the content of the variables are.

@4ian
Copy link
Owner

4ian commented Jan 23, 2019

I think this should be ready for a review then.

Cool, let me know if you can merge master in your branch, to see if the issue with the variable containing JSON being converted to JSON is fixed (and more generally, the issue of variable containing quotes, backslash, new lines being converted to JSON).

If you are ok with the use of the result variable, I'd start writing the wiki article.

Yes that's fine :)
A nice wiki article will be really useful for people to take fully advantage of this new extension.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 23, 2019

I checked the execution with your updated JSON serializer again:
grafik
Looks good 👍

@4ian
Copy link
Owner

4ian commented Jan 23, 2019

Nice! Let me review the code now

Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic of the code sounds good.

A few changes that I've noted though, that all boil down to keeping things consistent:

  • We should always specify JSON file or "file ... as JSON".
  • We should always write "scene variable". The fact is that the variable can be a structure, a number or a string, but this does not matter. This is especially important for users as they might be confused about is this a global, scene or object variable?
  • Name of the functions internally should also refer to variable, and not structure.
  • A few typo: finished, asynchronous - make sure to replace them in the whole file.

Hope it make sense :) Thanks for you work on this so far.

Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/JsExtension.js Outdated Show resolved Hide resolved
Extensions/FileSystem/filesystemtools.js Outdated Show resolved Hide resolved
Extensions/FileSystem/filesystemtools.js Outdated Show resolved Hide resolved
Extensions/FileSystem/filesystemtools.js Outdated Show resolved Hide resolved
Extensions/FileSystem/filesystemtools.js Outdated Show resolved Hide resolved
@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 24, 2019

The wiki article has been updated. I will try to resolve your findings this evening.

I'm lacking the rights to add the article about the file system extension (and screenshot extension as well) to the "all other features" article. The articles can currently only be opened via the link inside the IDE. Not even the search field in the wiki brought the articles up.
I think the file system article should be located directly next the storage article. I also added a comparison between the two so that it is easier for game developers to decide which one to use. If you find additional arguments feel free to add them.

And as always: If you are a native English speaker and want something to laugh about please proof read ;)

@4ian
Copy link
Owner

4ian commented Jan 24, 2019

Thanks for writing it!! :) Will add it to the all features page. Search index is updated everyday by Algolia afaik.

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 24, 2019

I guess that was all, thanks for reviewing 👍

@4ian
Copy link
Owner

4ian commented Jan 24, 2019

Cool, will merge if there is no major issues. Just started re-reading the File System wiki page. The Storage vs File system is a good idea and will surely be useful for a lot of users :)

@4ian
Copy link
Owner

4ian commented Jan 24, 2019

I've also added a few notes with links on the storage pages about Filesystem :)

@Wend1go
Copy link
Contributor Author

Wend1go commented Jan 24, 2019

One more thing before merging:
I only tested this on Linux. Would be cool if others could test it on Windows and MacOS too.

4ian added 2 commits January 24, 2019 21:56
* Fix an action wrongly mentioning JSON 
* Add "asynchronously" where needed in descriptions
* Always use scene variable for consistency, only mention structure as an information.
* Update name of functions to remove reference to structure, use variable instead as it's the type expected in the JS functions.
* Run Prettier
* Typo asyncrounousely => asynchronously
* `scenevar` is not a proper type in JavaScript, use `gdjs.Variable` instead
* Remove "optional" in documentation are these variables as not optional arguments in JavaScript.
@4ian
Copy link
Owner

4ian commented Jan 24, 2019

I've pushed a few fixes for typo and descriptions, ran Prettier on the files, changed mention to structure in the code by variable - while keeping a mention about structure in description of the actions so that it's clear that structure are properly converted to JSON and vice-versa.
Hopefully I've not broken anything!

@4ian 4ian merged commit fc3f8a9 into 4ian:master Jan 24, 2019
@4ian
Copy link
Owner

4ian commented Jan 24, 2019

Merged :)
Thanks a lot for your work on this 🎉 This will be super useful for people wanting to do advanced manipulation on files or save progress/settings in file on desktop.
Thanks for writing the wiki page though, really appreciate it - it's a key point for GDevelop to have everything documented 🙂

@4ian
Copy link
Owner

4ian commented Jan 25, 2019

Just released beta 63 with these new additions. If anyone on Windows can test would be great. I'm not worried at all though, Node.js apis are very portable and never had an issue with them on GDevelop itself.

@Bouh
Copy link
Collaborator

Bouh commented May 2, 2019

@Wend1go
PARAM1 at line 123 and 149 need underscore =)
https://github.com/4ian/GDevelop/blob/master/Extensions/FileSystem/JsExtension.js#L123

@Bouh
Copy link
Collaborator

Bouh commented May 6, 2019

@Wend1go maybe another thing.
I try to convert JSON files from Gdevelop in variable structure and save without modification.
And the result in output are different than input.
The events use gdjs.fileSystem.loadVariableFromJSONFile and gdjs.fileSystem.saveVariableToJSONFile
On my screen on left the original file.
On right the file loaded and saved with the events.
You can see at line 15 the array is different.
image

@4ian
Copy link
Owner

4ian commented May 6, 2019

This is due to lack of support for arrays in GDevelop structures. Arrays are converted to objects with keys "0", "1", "2"...
A potential improvement would be to store as array if the only keys are numerical keys starting from 0 and in proper order. (But this could also create unwanted resulsts).
The best improvement would be to actually add actions/conditions and slightly rework variables (and variable editor) to support arrays.

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.

3 participants