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 SaveWorld/LoadWorld schedules #66

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

johanhelsing
Copy link
Collaborator

Saving loading, same approach as in #52, but with checksums fixed.

Paves the way for more modular (e.g. Clone-based snapshotting).

@johanhelsing
Copy link
Collaborator Author

johanhelsing commented Oct 15, 2023

I want to get something similar to @bushrat011899's plugin-based snapshotting. Just thought it's good to get this more uncontroversial prep step in first.

Copy link
Owner

@gschup gschup left a comment

Choose a reason for hiding this comment

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

after merging #67 and #68, there now appears to be a merge conflict. I am sorry to inflict this on you, but it should be a simple fix 😅

@gschup
Copy link
Owner

gschup commented Oct 15, 2023

This is the part where I got a weird initialization bug in the rewrite branch. I don't remember a 100%, but the p2p example would sometimes crash due to some resources not being available (a schedule running before it should). For some reason, it happened only sometimes. We should check if the examples are working as intended.

@johanhelsing
Copy link
Collaborator Author

I only ran the box_game p2p test once and synctest once, that worked. I can also give it a go in my own game for a more complex test.

@johanhelsing
Copy link
Collaborator Author

after merging #67 and #68, there now appears to be a merge conflict. I am sorry to inflict this on you, but it should be a simple fix 😅

No problem :)

Copy link
Owner

@gschup gschup left a comment

Choose a reason for hiding this comment

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

All of this is in line with the ideas from #52, so I really cannot say anything. It also works on my machine. Thanks for doing this in a rigorous, orderly manner! Feel free to merge once you tried it with your game and are confident it works!

@johanhelsing
Copy link
Collaborator Author

One difference between this and #52, is that in #52 run_ggrs_schedules is added to the the Update schedule instead of PreUpdate... I'm not sure what is more correct, or whether it's the source of any of any issues in #52, but just thought I'd mention.

@gschup
Copy link
Owner

gschup commented Oct 19, 2023

@johanhelsing do you have any remaining concerns with this PR? I think this is ready to be merged after fixing the merge conflicts.

@johanhelsing
Copy link
Collaborator Author

johanhelsing commented Oct 19, 2023

@johanhelsing do you have any remaining concerns with this PR? I think this is ready to be merged after fixing the merge conflicts.

Yeah, I was holding off because I was uncertain about the desync detection issues in #71 (but that turned out to just be the detection that was broken and irrelevant to this PR). I've tested it with extreme bevy (and that works great), but not in cargo space.

Will rebase and merge

@gschup gschup merged commit 730d4d7 into gschup:main Oct 19, 2023
1 check passed
@bushrat011899 bushrat011899 mentioned this pull request Oct 19, 2023
4 tasks
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