Skip to content

Network logging improvements #406

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

helmutbuhler
Copy link

@helmutbuhler helmutbuhler commented Mar 11, 2025

Some improvements for developers involving network logging:

  • Show IP in player tooltip in DEBUG and INTERNAL builds
  • Add DEBUG_LOG message when sending packet is incomplete, when an unknown packet is received and when a packet cannot be sent.
  • Consistently print an IP, not just the hex value, in logs.

I also simplified some printing of IP addresses with a new macro PRINT_IP_HELPER. I realize we don't want big refactorings yet, but after expansion this macro should result in the exact same code as before, and it simplifies network debugging in the future. And when we clean up the string handling in the future, we can search for this macro and replace it with a functioncall.

@xezon
Copy link

xezon commented Mar 11, 2025

Could we split this perhaps in 2 Pull Requests? One for the fix and another for the logging?

@helmutbuhler
Copy link
Author

Hm, okay I'll do that. I was hoping this would be okay since the changes are pretty trivial.

…sn't include player name, only login and host)"

This reverts commit 3be730d.

# Conflicts:
#	GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanGameOptionsMenu.cpp
#	GeneralsMD/Code/GameEngine/Source/GameClient/GUI/GUICallbacks/Menus/LanLobbyMenu.cpp
@xezon
Copy link

xezon commented Mar 11, 2025

Yes size of this change is totally fine, but they do different things. Aka the logging is not needed if someone just wants the text fix.

@helmutbuhler helmutbuhler changed the title Lan player tooltip fix and minor LAN fixes Network logging improvements Mar 11, 2025
@helmutbuhler
Copy link
Author

Alright, I just removed the tooltip and LAN Serial change out of this PR and updated title and description

@xezon xezon added Network Anything related to network, servers Refactor Improves the structure of the code, with negligible changes in function. ZeroHour Relates to Zero Hour Debug Is mostly debug functionality labels Mar 11, 2025
@xezon
Copy link

xezon commented Mar 11, 2025

Looks good to me. Will this need a yaml documentation? Perhaps not because it is not user facing.

Not hitting the Approve button yet because we still hold off merging code changes.

@helmutbuhler
Copy link
Author

Nothing user facing here, so I don't think mention of this in the changelog is neccessary

@DevGeniusCode
Copy link

Not hitting the Approve button yet because we still hold off merging code changes.

In addition, it is worth checking before merging whether the feature can also be implemented for Generals, thus maintaining consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Debug Is mostly debug functionality Network Anything related to network, servers Refactor Improves the structure of the code, with negligible changes in function. ZeroHour Relates to Zero Hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants