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

feat: Improve console output to show packet enum names (preprocessor macro) #1340

Closed
wants to merge 14 commits into from

Conversation

jadebenn
Copy link
Collaborator

This PR uses a modified pre-processor macro taken from https://mariusbancila.ro/blog/2023/08/17/how-to-convert-an-enum-to-string-in-cpp/ in order to automatically generate matching to_string functions for the eGameMessageType and eWorldMessageType enums and use them to output the actual enum names to the console. In debugging, it makes it MUCH clearer which game messages are being recieved.

@jadebenn jadebenn changed the title Improve console output to show packet enum names feat: Improve console output to show packet enum names Dec 17, 2023
@Jettford
Copy link
Collaborator

What on god's green earth is this?

@EmosewaMC
Copy link
Collaborator

What on god's green earth is this?

I think the description is exactly what this is

@jadebenn
Copy link
Collaborator Author

Updated the code. Please let me know what you think.

change function handle

formatting and function definition tweaks
@jadebenn jadebenn force-pushed the ImproveConsoleOutput branch from 7d9cdcc to ffdc926 Compare December 17, 2023 23:38
@jadebenn jadebenn marked this pull request as ready for review December 18, 2023 06:29
@aronwk-aaron
Copy link
Member

Will have to say that I'm very much in favor of the concept, but having to change every enum we have defined is an absolute pain and makes maintaining them harder.
As @Jettford suggested: https://github.com/Neargye/magic_enum would be a preferable solution.
We would only have to edit logs for this to work and we can vendor the header for that library

@jadebenn
Copy link
Collaborator Author

Will have to say that I'm very much in favor of the concept, but having to change every enum we have defined is an absolute pain and makes maintaining them harder. As @Jettford suggested: https://github.com/Neargye/magic_enum would be a preferable solution. We would only have to edit logs for this to work and we can vendor the header for that library

I think there's a misunderstanding as to my intention here: I don't want to convert every enum, just the ones that need a string lookup function. In this case, that would be the two currently included. This could be expanded to more enums in the future, but I think it's unlikely we'll need to given the narrow use-case.

As to using magic enums, I did some reading on the documentation and I'm not sure how well it handles enums with more than 128 entries, as that behavior is disabled by default and has to be specifically enabled, with the documentation stating that it was originally designed for small enums. As eGameMessageTypes has well over a thousand entries, I'd be worried about the scalability.

Using preprocessor macros like this isn't my favorite thing in the world, but consider that the problem here is essentially one of code duplication: We need the same sequence of items to be defined identically in two different places in the code, once as an enum, and once as a vector. This can be done manually, but it's not very maintainable and very easy to get "out of sync." The macros here simply automate that process.

It's probably possible to make the StringifiedEnum::ToString() function a template and pull it out of the macro definitions and directly into StringifiedEnums.cpp, though I'm not sure that approach is necessarily superior to the current one. I unfortunately can't figure out any way to remove the necessity of macros for the creation of both the vector and enum, though. Not unless we do it by hand or redo the approach with magic enums as a dependency.

@jadebenn
Copy link
Collaborator Author

Actually, I take back what I said about being able to template-ify the ToString function. I don't think that's possible given the aforementioned need to pass it the same sequence of entries as are in the enum, which means it needs the macro. The good news is that function overloading takes care of any potential conflicts, though.

@jadebenn jadebenn changed the title feat: Improve console output to show packet enum names feat: Improve console output to show packet enum names (preprocessor macro method) Dec 20, 2023
@jadebenn
Copy link
Collaborator Author

jadebenn commented Dec 20, 2023

After further discussion with the other contributors, I have made a new PR utilizing magic_enum as a dependency instead.

@jadebenn jadebenn closed this Dec 20, 2023
@jadebenn jadebenn changed the title feat: Improve console output to show packet enum names (preprocessor macro method) feat: Improve console output to show packet enum names (C preprocessor) Dec 20, 2023
@jadebenn jadebenn changed the title feat: Improve console output to show packet enum names (C preprocessor) feat: Improve console output to show packet enum names (preprocessor macro) Dec 20, 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