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

Custom message improvements #2892

Merged

Conversation

leggettc18
Copy link
Contributor

@leggettc18 leggettc18 commented May 17, 2023

This PR contains what I think is a cleaner API for Custom Messages, and implements some move semantics so there should be fewer string allocations.

  • Implements a new CustomMessage class to replace the CustomMessageEntrys with something utilizing better object oriented design. This class should also properly utilize move semantics to prevent unnecessary string allocations when provided string literals or const char*s
  • Removes ReplaceStringInMessage methods from the CustomMessageManager and moves its functionality to the new CustomMessage class' Replace functions.
  • Adds a few other convenience methods and operators on CustomMessage, such as capitalization.
  • Cleans up areas where the CustomMessageManager was used, replacing some unnecessarily heap allocated structs with stack-allocated ones (i.e. std::vector to std::array, or moving an array into the function where it was used.

I would appreciate if someone could assist me in getting some sort of profiling going to verify that the claims about less allocation are true. I don't quite know how to check for that unfortunately.

Build Artifacts

Should result in much fewer allocations when rendering custom messages during a Randomizer playthrough.
This should allow for fewer allocations during randomizer seed generation and runtime, plus a couple of other enhancements that utilize custom messages. It does this by taking advantage of move semantics to prevent extraneous allocations when string literals are passed in. It also takes advantage of OOP to hopefully end up with a cleaner API.
Copy link
Contributor

@briaguya-ai briaguya-ai left a comment

Choose a reason for hiding this comment

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

Overall this looks great! Assuming this has been tested I'm close to saying :shipit:!

Left one little naming comment in there but otherwise this is awesome!

@leggettc18
Copy link
Contributor Author

Assuming I'm using Visual Studio's profiler correctly, after seed generation we seem to have dropped about 200 string allocations with this PR.

@leggettc18 leggettc18 merged commit f2f5a75 into HarbourMasters:develop May 18, 2023
@garrettjoecox garrettjoecox added this to the Sulu (7.1.x) milestone Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants