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

Refactor: Remove unnecessary parameter avatar #76498

Merged
merged 14 commits into from
Sep 19, 2024

Conversation

Brambor
Copy link
Contributor

@Brambor Brambor commented Sep 17, 2024

Summary

None

Purpose of change

There are a lot of menus that take avatar as an argument. They are menus, so there is no need for them to ask for the avatar in parameters since they can just get_avatar(). Other Character than the avatar can never open menus, since they are not a player.

The benefit of removing the avatar from the parameter is that the callee may not need to #include avatar.h. Didn't happen, unfortunately.

Found some dead code as well.

Describe the solution

Remove parameter avatar where possible within src/game_menus.h

Describe alternatives you've considered

Remove some Character too, since it always probably is an avatar. I want this as simple as possible, since I don't want to do anything more with this.

Testing

Compiles?

Additional context

@Brambor Brambor marked this pull request as ready for review September 17, 2024 15:28
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` Melee Melee weapons, tactics, techniques, reach attack json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Sep 17, 2024
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Sep 17, 2024
@Brambor Brambor closed this Sep 17, 2024
@Brambor Brambor reopened this Sep 17, 2024
@Brambor Brambor closed this Sep 17, 2024
@Brambor Brambor reopened this Sep 17, 2024
@Brambor Brambor changed the title Rm parameter avatar Remove unnecessary parameter avatar Sep 17, 2024
@Brambor Brambor changed the title Remove unnecessary parameter avatar Remove unnecessary parameter avatar Sep 17, 2024
@Brambor Brambor changed the title Remove unnecessary parameter avatar Refactor: Remove unnecessary parameter avatar Sep 18, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Sep 18, 2024
@Maleclypse Maleclypse merged commit edd4099 into CleverRaven:master Sep 19, 2024
61 of 79 checks passed
@Brambor
Copy link
Contributor Author

Brambor commented Sep 20, 2024

Finding the PR that stopped using holster_inventory_preset with git bisect run (for my future reference):

holster_inventory_preset is a class defined in a .cpp file, so all references are within that file. The class definition contains the text holster_inventory_preset twice (on separate lines too), there is no other mention of it in the whole file. My current master is d5e57fa. If we go 25000 commits back to a5904e6, we can see the text holster_inventory_preset used a third time, that is the usage of that class, so it was useful back then. We want to see when it went from 3 usages to 2.

Bash script: cat src/game_inventory.cpp | grep holster_inventory_preset | wc -l shows the number of lines the text holster_inventory_preset is on. Make a file named tst with the following: contents

tst

lines=$( cat src/game_inventory.cpp | grep holster_inventory_preset | wc -l )

if [ $lines -eq 2 ]; then
  exit 1
fi

This script returns 1 if the number of lines is equal to 2. Otherwise returns 0. This corresponds to bad when the script returns 1 and good when it returns 0. Test the script for known good and bad revisions: In bash: ./tst; echo $? shows 1 on master and 0 on master~25000.

Now do the following:

# checkout known good commit
git checkout a5904e602d
bit bisect start
git bisect good head
git bisect bad master

git bisect run ./tst
Sample output
C:\GitHub\Cataclysm-DDA>git bisect start

C:\GitHub\Cataclysm-DDA>git bisect good head

C:\GitHub\Cataclysm-DDA>git bisect bad master
Bisecting: 23390 revisions left to test after this (roughly 15 steps)
[85a9936e7c864144392b59a63725b1c2db68933f] Merge pull request #51167 from Night-Pryanik/wyrms-mine-jsonify

C:\GitHub\Cataclysm-DDA>git bisect run ./tst
running  './tst'
Bisecting: 11681 revisions left to test after this (roughly 14 steps)
[8cea0fce70c87ea93c6fd4e409c68558e24ce42e] Merge pull request #40023 from Qrox/uilist
running  './tst'
Bisecting: 5840 revisions left to test after this (roughly 13 steps)
[522685c9cf84d780be199f83cbdb7e411ec8bf54] Merge branch 'master' into Shoobieclysm
running  './tst'
Bisecting: 2919 revisions left to test after this (roughly 12 steps)
[1ac8ab08a52f47a375e5f917d9959d4db9a3ba07] Merge pull request #42419 from Jerimee/patch-8
running  './tst'
Bisecting: 1460 revisions left to test after this (roughly 11 steps)
[a64d7d04e94e650e9ef6150c529b3565c598e0a5] Use arrays in zh_TW.names json
running  './tst'
Bisecting: 731 revisions left to test after this (roughly 10 steps)
[8f142b41ec6ef90b7fe9a36b92ba723fc700eef1] adjust item_location test to use new obtain_cost algorithm
running  './tst'
Bisecting: 364 revisions left to test after this (roughly 9 steps)
[687ca3f4d7be35a380e266d0aa6e311751424806] Merge pull request #40264 from ymber/fix_mags
running  './tst'
Bisecting: 180 revisions left to test after this (roughly 8 steps)
[ecc602c0dd2d91610c81dae0952f0712f607966f] Merge remote-tracking branch 'origin/pr/39406'
running  './tst'
Bisecting: 90 revisions left to test after this (roughly 7 steps)
[10a446fb68f29dc9b789b7d3644fb4e18e614a48] Revert "fix vehicle power test"
running  './tst'
Bisecting: 45 revisions left to test after this (roughly 6 steps)
[c9eeef5e4e2c964c9132c6bf98f8063b1e44f83f] fix crafting tests
running  './tst'
Bisecting: 22 revisions left to test after this (roughly 5 steps)
[28947bddf8fada97fe14828084f5d1090bc5b32c]  remove superfluous call to seal() in wish.cpp
running  './tst'
Bisecting: 10 revisions left to test after this (roughly 4 steps)
[39e27c9969e862bb2408e4d2089f3d96de1cb506] fix professions loading inventory incorrectly
running  './tst'
Bisecting: 5 revisions left to test after this (roughly 3 steps)
[380ed810613fd39beb68c8ac916769875995c224] remove unneeded backpack logic from drop activity
running  './tst'
Bisecting: 2 revisions left to test after this (roughly 1 step)
[f9c60d0cfa4f8076373472c4f79a56f1bc6c9e28] nested containers json
running  './tst'
Bisecting: 0 revisions left to test after this (roughly 1 step)
[5383aaf6ebafab59d15e160b4e85fffae59eead5] change inventory UI to show items in containers
running  './tst'
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[0ee9b4a6e246fcb48b8de6d910ade4a3709208f9] markdown documentation of pocket data json
running  './tst'
5383aaf6ebafab59d15e160b4e85fffae59eead5 is the first bad commit
commit 5383aaf6ebafab59d15e160b4e85fffae59eead5
Author: KorGgenT <[email protected]>
Date:   Sat Apr 11 19:23:24 2020 -0400

    change inventory UI to show items in containers

 src/game_inventory.cpp | 108 +++++++++++++++++++++++++++++++------------------
 src/game_inventory.h   |   5 ++-
 src/inventory_ui.cpp   | 102 ++++++++++++++++++++++++++++++++++++++--------
 src/inventory_ui.h     |   4 +-
 4 files changed, 161 insertions(+), 58 deletions(-)
bisect found first bad commit

We have found that 5383aaf is the culprit.

We can search GitHub to find the associated PR: https://github.com/cleverRaven/Cataclysm-DDA/issues?q=5383aaf6ebafab . There we make sure that it was an oversight and that the class definition can be safely deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions Melee Melee weapons, tactics, techniques, reach attack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants