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

Expose a File.flush() method to scripting #44396

Merged
merged 1 commit into from
Feb 13, 2021

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Dec 15, 2020

Follow-up to #44393.

This can be used to ensure a file has its contents saved even if the project crashes or is killed by the user (among other use cases).

Note: I couldn't test this as I get "File must be opened before use." when trying to create a new file following the File code sample in the class reference, even with a vanilla master build.

Edit: Using File.WRITE instead of File.READ_WRITE works for testing.

See discussion in #29075.

@Calinou Calinou requested a review from a team as a code owner December 15, 2020 14:32
@Calinou Calinou added cherrypick:3.x Considered for cherry-picking into a future 3.x release enhancement topic:core labels Dec 15, 2020
@Calinou Calinou added this to the 4.0 milestone Dec 15, 2020
@Xrayez
Copy link
Contributor

Xrayez commented Dec 15, 2020

I think this is certainly useful for things like storing game replays where you can continuously write to disk (at some fixed steps via chunks of data, of course), or saving procedurally generated world.

@andy-noisyduck
Copy link
Contributor

andy-noisyduck commented Dec 15, 2020

Calling flush() on a file that doesn't support flushing (i.e. a file opened with open_compressed, or open_encrypted) will be silently ignored. Is this the desired behaviour?

Edit: It seems the behaviour isn't consistent between the types. A packed file (PackedData) will error if flush is called. Calling flush on a compressed will error if it was opened as a zip, but fail silently if another compression scheme is used.

These problems are existing and not caused by this PR - though previously you couldn't trigger them in scripts due to the lack of the bind. Perhaps this is fine (it's certainly better than it was), and we should just open another ticket for the correction.

@Xrayez
Copy link
Contributor

Xrayez commented Dec 15, 2020

Those problems should be treated on the same level, regardless of whether the functionality is exposed (because it's possible to do the same thing via C++ modules currently), so I'm for consistency in this case (and yes, if that's the case, people will report more issues with this, but I don't see nothing wrong with that).

Calling flush() on a file that doesn't support flushing (i.e. a file opened with open_compressed, or open_encrypted) will be silently ignored. Is this the desired behaviour?

I wonder what would be the alternative way to write compressed data? Perhaps the partial data/buffer should be manually compressed before write in this case (via PackedByteArray.compress())?

@akien-mga
Copy link
Member

Note: I couldn't test this as I get "File must be opened before use." when trying to create a new file following the File code sample in the class reference, even with a vanilla master build.

Does it work in 3.2?

@akien-mga
Copy link
Member

Note: I couldn't test this as I get "File must be opened before use." when trying to create a new file following the File code sample in the class reference, even with a vanilla master build.

Does it work in 3.2?

Bump. Could also use a rebase to make sure it builds fine after various refactorings.

@Calinou Calinou force-pushed the add-file-flush-method branch from b7164a2 to 4d617fc Compare February 13, 2021 00:31
@Calinou
Copy link
Member Author

Calinou commented Feb 13, 2021

Rebased and tested on master and 3.2, it works successfully. I've also added more documentation about flushing and closing files.

@Calinou Calinou force-pushed the add-file-flush-method branch from 4d617fc to d7d4345 Compare February 13, 2021 00:35
This can be used to ensure a file has its contents saved
even if the project crashes or is killed by the user
(among other use cases).

See discussion in godotengine#29075.
@Calinou Calinou force-pushed the add-file-flush-method branch from d7d4345 to ab39746 Compare February 13, 2021 00:37
@akien-mga akien-mga merged commit 041cccc into godotengine:master Feb 13, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.2.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 14, 2021
@Calinou Calinou deleted the add-file-flush-method branch March 31, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants