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

[Port] Add in-game progression diary #1965

Merged
merged 39 commits into from
Oct 30, 2022

Conversation

leoCottret
Copy link
Collaborator

@leoCottret leoCottret commented Oct 6, 2022

Summary

Purpose of change

Fixes #1917
An in-game way to take notes was something missing in the game, so when I saw that someone already made it in DDA (+extra features), I just had to port it

Describe the solution

Port the diary
CleverRaven/Cataclysm-DDA#52372 (Original PR)
CleverRaven/Cataclysm-DDA#53576 (Fix typos)
CleverRaven/Cataclysm-DDA#55045 (INFO menu option)
CleverRaven/Cataclysm-DDA#53582 (Better translation support for head text)
CleverRaven/Cataclysm-DDA#54720 (Clean code, fix typos)
CleverRaven/Cataclysm-DDA#56253 (Add more robust code that fixes several bugs)
CleverRaven/Cataclysm-DDA#57321 (Change the pop op after an edit to Yes by default)

A few personal modifications

  • 1 - Fix the head text bugging through the second diary page on small to medium screens
  • 2 - Fix the Info display on small screens (phones) by allowing up to 3 lines to be displayed
  • 3 - Complete rework of UI, removing the padding to improve lisibility and maximizing the used space (all screens)
    Comparison between the current BN and DDA versions
    image
  • 4 - Add a maximum width limit and centering the UI after reaching the limit (medium to big screens)
  • 5 - Replace the time since last entry by a shorter, universal (no translation needed for hours/minutes/seconds) and more precise version

At the limit
image

Above the limit
image

  • 6 - Fix many typos and viariable names that didn't use snake case
  • 7 - Add martial arts progression tracking (what martial arts are known at the start, then the new ones learnt)
  • 8 - Change export format from .txt to .md. .md is for markdown and it's basically readable by everything that can read .txt. With this I could add a way to better seperate the entries in the exported file
  • 9 - Add maximum bionic power progression tracking

Testing

I did the same tests as in the original PR:

  • create, edit an entry
  • create, delete a page
  • export the diray
  • save and load
  • resize the game window
  • die and write last entry
  • verify that the progression is tracked
    (Since last changes)
  • try to go outside the window and find a way to crash the game
  • write special chars and save/load game, export diary

@leoCottret leoCottret changed the title Add in-game progression diary [Port] Add in-game progression diary Oct 6, 2022
@leoCottret
Copy link
Collaborator Author

Will fix clang-tidy later

@olanti-p
Copy link
Contributor

olanti-p commented Oct 9, 2022

Some issues with the implementation:

  • The header fix doesn't work on my end (visual studio build, default-ish graphic settings, 192x49 cells terminal size)
    image

  • The diary and UI elements are extremely tiny on small window sizes, yet there is free space available around them. Would it be possible to adjust the sizing/positioning code to get rid of the transparent padding on small window sizes? Also, the Info window on the bottom is malformed here.
    image

  • There are weird issues with cursor positioning, namely it seems to be possible to position it outside text. Repro steps:

    1. Open diary
    2. Create new page
    3. Press Enter to start editing
    4. Type asd
    5. Press right arrow
  • I managed to crash it by typing some text, then erasing it, then pressing "down".
    image

  • There were a couple followup bugfixing PRs that may solve some of the issues listed above:

@leoCottret
Copy link
Collaborator Author

  • About the header fix, it's not responsive for now, so I'll see if i can make it responsive, otherwise I'll just remove it and leave it like in DDA
  • About the look in small windows, I'm not sure it can be that much improved, here is what it looks like in DDA after dozens(?) of modifications
    image
    Unless we implement a complete new UI for phones?
  • The crash and cursor positionning are annoying though, I'll look into that

This PR was more to add a working first version of the diary (quick win). If we see that our player base like it much and mind the small UI problems, we can then improve it further.

@leoCottret leoCottret marked this pull request as draft October 9, 2022 21:49
@leoCottret leoCottret requested a review from olanti-p October 13, 2022 03:35
@leoCottret
Copy link
Collaborator Author

Ready for review, I updated the description with the new changes

@Coolthulhu Coolthulhu self-assigned this Oct 15, 2022
@@ -363,6 +364,14 @@ int avatar::time_to_read( const item &book, const player &reader, const player *
return retval;
}

diary *avatar::get_avatar_diary()
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to initialize the diary on construction and then return a proper reference to it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave this one unresolved since we agreed on Discord to leave it like that for now, but it would be great if someone with better cpp skills took the time to take care of it.

src/avatar.h Outdated Show resolved Hide resolved
src/avatar.h Outdated Show resolved Hide resolved
src/diary.h Outdated Show resolved Hide resolved
src/diary.h Outdated Show resolved Hide resolved
src/diary.h Outdated Show resolved Hide resolved
src/kill_tracker.h Outdated Show resolved Hide resolved
@scarf005 scarf005 added src changes related to source code. JSON related to game datas in JSON format. labels Oct 26, 2022
@Coolthulhu Coolthulhu merged commit 6dd7a31 into cataclysmbnteam:upload Oct 30, 2022
@Coolthulhu
Copy link
Member

I noticed that "since last entry" can "understand" when an entry was deleted. That's good.
It doesn't note mutations. That's a pretty big change to character, so it's weird.

@leoCottret
Copy link
Collaborator Author

Well it should, there's all the code for it, maybe it's a bug due to what I reported to you?
Initial mutations
image
Gained and lost mutations
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port request: progression diary
4 participants