Smart all the Things! #5022
Closed
Smart all the Things! #5022
+6,310
−7,123
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: ramon-bernardo <[email protected]>
Co-authored-by: ramon-bernardo <[email protected]>
…ameter Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Rename loop variable to avoid parameter shadowing
…, remove fail-fast Co-authored-by: ranisalt <[email protected]>
Co-authored-by: ranisalt <[email protected]>
Co-authored-by: ranisalt <[email protected]>
Refactor code per PR #11 review feedback
Use weak_ptr for non-owning attributes
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| g_game.browseFields.erase(getTile()); | ||
|
|
||
| for (Item* item : itemlist) { | ||
| const auto& parent = getParent(); |
Check warning
Code scanning / CodeQL
Virtual call from constructor or destructor
Contributor
|
Close #2238 |
Co-authored-by: Ramon Bernardo <[email protected]>
3 tasks
Contributor
|
@ranisalt |
Member
Author
|
@gesior the branch is ready to be tested and the server is (seems to be) working properly |
Contributor
|
I am daily running some tests while I test systems that I am working on in parallel, everything seems to be working perfectly |
Contributor
|
As soon as I have a reasonably good amount of time, I will check all the changes and try to find possible parts of the code that still need to be adapted |
Member
Author
|
I tried my best |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Prelude
Changes Proposed
This pull request introduces several changes to the code in order to migrate
Thingand it's derived classes to usestd::shared_ptrand to remove ofCylinderas an utility class. These updates improve memory safety and potentially fix multiple dangling pointers and memory leaks.Note: according to basic testing, memory usage increases by about 30% using the default map (138 MB vs 179 MB). Still need to benchmark against a larger map to grab better numbers.
Also, the commits and files tabs are massive, don't try to open them in a slow computer 😅
Co-authored-by: @ramon-bernardo
Issues addressed:
Fixes #2238
Fixes #2260
Fixes #2682
Fixes #3552
Fixes #4752
Fixes #4961
Closes #2193
Closes #2219
Closes #5019
(needs to be confirmed)
How to test:
Just run the server, everything should be unchanged from the client perspective.