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

Update last_world_pos considerations #2838

Merged
merged 2 commits into from
May 19, 2023

Conversation

KheirFerrum
Copy link
Collaborator

@KheirFerrum KheirFerrum commented May 18, 2023

Summary

SUMMARY: Bugfixes "Update last_world_pos considerations"

Purpose of change

Several issues noted with current version, namely

  • Saving the game and returning to the main menu does not update last_world_name properly
  • last_world_pos returns wrong offset on deleting/creating a world.

First issue has been solved on DDA but I was unable to git blame the commit, though I was able to find the solution code. If someone is willing to step forward with the commit I will add them as co-author.

Second issue was solved on DDA as well, but produced more issues and in my opinion was inefficient.

In general, I believe that derived indexes should only be regenerated instead of modified, as this prevents any risk of segfaults and keeps the code easy to follow.

In a similar vein, I don't believe that such indexes should be saved as class variables, as you will need to remember to update them every single time you code for dependent value changes instead of letting it be regenerated.

Describe the solution

Added code to update last_world_name and last_character_name on saving the game.

Added get_world_index() function using existing std::find code. Now last_world_pos is updated every redraw. The effect of this on performance should be negligible on any half decent phone/computer.

Describe alternatives you've considered

  • Regenerate last_world_pos only when world is created/destroyed.
    • Would be more performant, but as I consider the boost to be generally negligible, I've elected against it.

Testing

  • Create 5 worlds, and make a character in the third world. Save and return to the main menu.
  • Check that pointer when in load menu defaults to the third world. Move to world tab and delete either world 1/2
  • Check that it still defaults to the correct world when in load menu. Move to world tab and delete one of the last 2 worlds
  • Check that it still defaults to the correct world when in load menu.
  • Add worlds above and below the correct world (order is alphabetical). Check that it still defaults to correct world in load menu.

Additional context

On saving the game it will be updated.

Will no longer provide wrong offsets after save/load.
@github-actions github-actions bot added the src changes related to source code. label May 18, 2023
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

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

Tests

Create 5 worlds, and make a character in the third world. Save and return to the main menu.
Check that pointer when in load menu defaults to the third world.

image
created a new character in TestA

Move to world tab and delete either world 1/2
Check that it still defaults to the correct world when in load menu.

image
after removing world Event

Move to world tab and delete one of the last 2 worlds
Check that it still defaults to the correct world when in load menu.

image
after removing world TestB

Add worlds above
image

after adding world asdf

and below the correct world (order is alphabetical). Check that it still defaults to correct world in load menu.
image
after adding world zzz

LGTM

@scarf005 scarf005 merged commit 0f956fa into cataclysmbnteam:upload May 19, 2023
@KheirFerrum KheirFerrum deleted the last_world_pos-fix branch May 19, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants