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

Save and load the current planned hyperjump in the save file #5721

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

JonBooth78
Copy link
Contributor

The planned hyperspace route, with all stops is saved along with the save game, and restored on load.

@impaktor
Copy link
Member

This fixes #5178 ?

@JonBooth78
Copy link
Contributor Author

@impaktor , only half of it; I'll get onto the rest later - thanks for pointing it out

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

Especially given that you're planning on serializing more sector view state, I'd recommend that you marshal the serialized data through a Serializer:Register() call in map-sector-view.lua instead of registering the serialization callback in hyperjump-planner.lua.

The hyperjump planner / sector view split is slightly unconventional / unfinished in that the hyperjump planner is not quite a re-usable class that the sector map owns an instance of (yet), but it's close enough that I'd recommend applying that same mental model.

This is not strictly required, but it will make it easier to clean up / finish encapsulating the hyperjump planner widget in the future.

@sturnclaw
Copy link
Member

Hmm... the more I look at this code the more I think the serialization should be entirely handled in map-sector-view.lua. 99% of this code is manipulating the C++ sector view object itself, and the only thing that involves the hyperjump planner widget is a one-off call in an onGameStart event to hyperjumpPlanner.buildRouteList().

@JonBooth78
Copy link
Contributor Author

@Web-eWorks , I think your right; I also suspect that the call to hyperjumpPlanner.buildRouteList() is also redundant as I think it gets called on entry to the screen; I just wanted to be super-safe.

I felt that as this was all to do with the route-planner, it made sense to live in here, although I agree, it really has little to do with that element, which is a UX one.

I'll have a crack at moving it to the suggested place and add the other missing bits of #5187

Before if the options to draw out of range labels was ticket, they were hidden and unticled they were shown.
@JonBooth78 JonBooth78 force-pushed the save-hyperspace-route branch from 7bd2722 to 1d1bb7d Compare January 28, 2024 09:17
@JonBooth78
Copy link
Contributor Author

@Web-eWorks , ready for re-review, all seems to be complete and working, with a bonus bug-fix thrown in at the end :-)

@JonBooth78
Copy link
Contributor Author

oh snap, take that back found a bug

@JonBooth78 JonBooth78 marked this pull request as draft January 28, 2024 09:20
@JonBooth78 JonBooth78 force-pushed the save-hyperspace-route branch from e091ba7 to d018a27 Compare January 28, 2024 09:33
@JonBooth78 JonBooth78 marked this pull request as ready for review January 28, 2024 09:34
@JonBooth78
Copy link
Contributor Author

OK, fixed that bug (doesn't look like I introduced it, oddly) and then clang formatted a file I touched on the way through. @Web-eWorks now ready to go

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

Two very minor tiny nitpicks but the code looks good otherwise. Haven't had time to test this branch, so it's provisionally approved pending a playtest sometime in the next 24 hours.

data/pigui/views/map-sector-view.lua Show resolved Hide resolved
src/SectorView.cpp Show resolved Hide resolved
@sturnclaw
Copy link
Member

OK, fixed that bug (doesn't look like I introduced it, oddly) and then clang formatted a file I touched on the way through.

This is quite the ancient bug oddly enough (4 years old!) but was completely benign up until yesterday when I merged #5724. I had the feeling something was odd with the onEnterSystem callback signature but rationalized it away due to lack of time. Thanks for catching that!

@sturnclaw sturnclaw merged commit 72bd4ad into pioneerspacesim:master Jan 29, 2024
6 checks passed
@JonBooth78 JonBooth78 deleted the save-hyperspace-route branch February 3, 2024 20:02
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