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

Allow renaming project assets while preserving id #576

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

chrismaltby
Copy link
Owner

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Attempt to fix issues seen in When renaming a background it corrupts the project. #574 where renaming assets causes ids to be regenerated and any scenes/events referring to those ids would be broken.

  • What is the current behavior? (You can also link to an open issue here)
    Asset file watching is handled in https://github.com/chrismaltby/gb-studio/blob/v2beta/src/lib/project/watchProject.js using the module chokidar which handles the complexities of cross platform file watching. In chokidar when a watched file is renamed it fires an unlink (delete) file event followed by an add file event with no way to determine that this was a single rename action (and it looks unlikely that this functionality will change Add a file "rename" event paulmillr/chokidar#303). This caused a rename to first delete the asset, then it will be readded, at which point it gets a new id and all previous references are broken.

  • What is the new behavior (if this is a feature change)?
    Using fs.stat I am fetching the inode value for each asset which appears to stay the same after a rename, when a file is deleted I store the asset data in a lookup table by inode and when a new file is added I check if the inode matches a recently deleted one and if it does I reuse the previous id and any additional previous data that might be useful to keep (such as the music speed conversion flag). With this change renaming an asset has a small flash of the asset being deleted before it reappears again a second a later with the scene collisions and actors positions unchanged.

As the inode values also get written to disk in the GBSProj file it's also now possible to rename an asset while GB Studio is closed and it will fix the links when you next load the project.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

Maybe, I've only tested this on Mac so far. I'm interested in seeing if this works correctly on Windows. I believe Windows uses 64-bit inode values so I'm using the bigint arg when calling stat await stat(filename, { bigint: true }); and storing the value as a string (since JSON doesn't support 64-bit values), without testing I can't be sure this is working. I'll do plenty before merging this.

Linux should be okay but also needs a lot of testing.

@RichardULZ
Copy link
Contributor

Awesome, should fix #387 too (which was not crashing in 2.0beta, but still had issues)
Will have to try these out.
I don't think we've ever fully squashed all the asset reload bugs for everyone, just moved them around, as they are so hard to track down when they do happen (haven't seen any issues so far, but pretty sure there's at least one user on beta that has...) could be a further issue with dropbox or something.

@chrismaltby
Copy link
Owner Author

@RichardULZ I've just done some testing on Windows and so far this seems to be working pretty well. The only issue I came across was Music files sometimes working and sometimes not, and I tracked it down to a one line config issue which was sometimes causing the add file event to fire before the delete file event because I wasn't polling the file for changes for long enough,.

I've just fixed this in the latest commit so now whenever a music file is changed it checks every 100 milliseconds to make sure no further changes happen until at least 5 seconds have passed using the awaitWriteFinish options in chokidar https://github.com/paulmillr/chokidar#performance I could probably reduce this time (it only waits one second for image assets) but I figured it doesn't need to be anywhere near as instant to update the music so it's probably better to err on the side of caution.

I need to test if this fixes #390 too. It's possible this will help there.

@chrismaltby chrismaltby merged commit 23ea50b into v2beta Sep 29, 2020
@chrismaltby chrismaltby deleted the fix/rename_assets branch September 29, 2020 19:10
@chrismaltby
Copy link
Owner Author

I've tested this on Windows, Mac and Linux now and it seems to be working well on all three!

Also from what I can tell it does seem to fix both #387 (undoing a deleted file restores the links in GB Studio) and #390 (So far been unable to get OpenMPT to break music links with this build)

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