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

Snapshot Save Single Action #7500

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

NeylMahfouf2608
Copy link
Collaborator

Save and/or load the whole game using the updateFromNetworkSyncData/getNetworkSyncData methods to write/read a complete JSON. Done in one action for each (at the moment, might change). Right now it writes both on a localFile and localStorage of the browser.

@NeylMahfouf2608 NeylMahfouf2608 requested a review from 4ian as a code owner March 24, 2025 16:28
_('Save the whole game'),
_('Save the wole game.'),
_('Syncall'),
'res/conditions/animation24.png',
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this :)

@@ -70,6 +70,44 @@ namespace gdjs {
} else {
logger.error('Unrecognized change in scene stack: ' + request);
}

const allSyncData: GameSaveState = JSON.parse(
Copy link
Owner

@4ian 4ian Mar 25, 2025

Choose a reason for hiding this comment

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

You do this getItem and parse at every frame, this seems wasteful for the CPU because it's then immediately discarded in almost all cases (i.e: when a loading is not requested).

@@ -243,9 +243,10 @@ namespace gdjs {
}

updateFromNetworkSyncData(
networkSyncData: TextObjectNetworkSyncData
networkSyncData: TextObjectNetworkSyncData,
options?: { skipMultiplayerInstructions: boolean }
Copy link
Owner

Choose a reason for hiding this comment

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

Discussed with you on Discord a while ago: all these options can probably be set in a type UpdateFromNetworkSyncDataOptions

);

if (currentScene._isLoadingRequested && allSyncData) {
currentScene
Copy link
Owner

Choose a reason for hiding this comment

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

That's a lot of logic => Put it in a method with a name that describe what this does.

);
return;
}
if (!options) {
Copy link
Owner

Choose a reason for hiding this comment

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

Relying only on the existence of options, and not reading a boolean with a proper name, is risky

localStorage.getItem('save') as string
);

if (currentScene._isLoadingRequested && allSyncData) {
Copy link
Owner

Choose a reason for hiding this comment

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

Don't access private properties (_somethingLikeThis) outside of the class they belong to.

Comment on lines 84 to 85
currentScene
.getGame()
Copy link
Owner

Choose a reason for hiding this comment

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

You're using currentScen.getGame() 3 times in this function. That should ring a bell :)

…added some constants in separated files for clarity
objectStoreName: string,
key: string
): Promise<any> {
if (!indexedDB) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's a good check, though what if indexedDB.open() (line below) fails? (if Safari does a weird thing for instance?)

My suggestion was more to do a try catch around everything to catch any weird stuff we didn't handle

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