Skip to content

add goal field to scenarios #450

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

Merged
merged 9 commits into from
Jun 22, 2022
Merged

add goal field to scenarios #450

merged 9 commits into from
Jun 22, 2022

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Jun 20, 2022

If there is a goal, immediately pop up a dialog with the goal, then
allow showing it any time afterwards with ^G.

See #356.

If there is a goal, immediately pop up a dialog with the goal, then
allow showing it any time afterwards with ^G.

See #356.
@byorgey
Copy link
Member Author

byorgey commented Jun 20, 2022

Note this suffers from a bug where popping up the scenario goal dialog autopauses the game but then it is not unpaused when closing the dialog. I think that bug already existed before, and is also being addressed in #441 . Not quite sure what the best way forward is.

@byorgey byorgey mentioned this pull request Jun 20, 2022
@TristanCacqueray
Copy link
Collaborator

I've proposed the modal fix separately in #452

@byorgey
Copy link
Member Author

byorgey commented Jun 20, 2022

Also, note that I intentionally did not update any of the scenarios in data/scenario to have a goal, to avoid causing merge conflicts with #411 . If you want to test this out, you can just temporarily edit one of the scenarios to have a goal field containing a list (similar to description).

@byorgey
Copy link
Member Author

byorgey commented Jun 21, 2022

@xsebek , thanks for the feedback, I think this is now ready again.

-- first loading entities and recipies from disk.
initGameState :: Maybe Seed -> Maybe String -> Maybe String -> ExceptT Text IO GameState
initGameState cmdlineSeed scenarioToLoad toRun = do
maxMessageQueueSize :: Int
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't change anything from here down to Initialization, I just moved it to try to organize the code a bit better.

@xsebek
Copy link
Member

xsebek commented Jun 21, 2022

Also, note that I intentionally did not update any of the scenarios in data/scenario to have a goal, to avoid causing merge conflicts with #411

Thanks, @byorgey, but I did not touch any Challenges so those can get the new goal right now 😉
Feel free to merge my PR #456 to this branch 😇

scenario <- loadScenario (fromMaybe "00-classic" scenarioName) (gs ^. entityMap)
liftIO $ scenarioToAppState scenario userSeed toRun $ AppState gs ui

-- XXX do we need to keep an old entity map around???
Copy link
Member

Choose a reason for hiding this comment

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

I do not think so, we can always reload it if we wanted to say compare it before saving the game (or just save the whole map). Do you have some use cases in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm worried that when we load a scenario with custom entities, they get permanently added to the entity map...

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that's unrelated to this PR though, and it would have already been an issue before. I'll think about it later.

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Jun 22, 2022
@mergify mergify bot merged commit 7b9d842 into main Jun 22, 2022
@mergify mergify bot deleted the scenario-goal branch June 22, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants