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

PR for the netplay developement! #2

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

Conversation

Mipppy
Copy link
Contributor

@Mipppy Mipppy commented Sep 8, 2024

Right now, there is working hosting and joining system. I'm joining to work on the group memory editing next, then probably cheat codes.

…n it on different devices

Main.py file will be removed eventually
…xt up: Polishing the netplay system and actually making the basics of changing memory together. Oh the fun to be had!
@Mipppy
Copy link
Contributor Author

Mipppy commented Sep 8, 2024

I think I just resolved the merge conflict correctly, you should probably check it though.

…l allow for more dynamic changing of messages. I also worked on netplay a bit and now the room and hosting system is complete. I'll spend some time moving all the netplay log messages over to the new file, and then its time for the actual feature of netplay! Fun awaits
@Mipppy Mipppy marked this pull request as draft September 9, 2024 01:07
Mipppy and others added 3 commits September 9, 2024 00:10
I was on my awful laptop that can't compile/build quickly, which is really useful for making the netplay
…t! The entire host/lobby system is ironed out and working well.
@@ -3,7 +3,7 @@
{
"buildCommandArgs": "-v",
"buildRoot": "${projectDir}\\out\\build\\${name}",
"cmakeCommandArgs": "-DCMAKE_TOOLCHAIN_FILE=C:\\vcpkg\\scripts\\buildsystems\\vcpkg.cmake",
"cmakeCommandArgs": "-DCMAKE_TOOLCHAIN_FILE=C:\\Users\\Tim\\vcpkg\\scripts\\buildsystems\\vcpkg.cmake",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the CMakeSettings.json shouldn't be tracked in git. Consider removing it since you're hard-coding your user file path which will only be valid for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed this last night after I was checking the Pr myself. I was afraid to straight up delete the file because I thought it might delete it locally. I’m not too familiar with git, so it would nice if you could tell me how I should resolve this. Thanks!

Copy link
Collaborator

@BullyWiiPlaza BullyWiiPlaza Sep 11, 2024

Choose a reason for hiding this comment

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

Yes, you can remove the file from future tracking, e.g. via this answer: https://stackoverflow.com/a/1274447
However, this will not remove the file from the commit history.

You could retroactively delete it from the history (preferred for unwantedly committed files): https://stackoverflow.com/a/64563565

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

// Accepts as many const char* arguments as you want, but it is highly recommended (Needed) that you use the same number of arguments as the LogMessage you choose corresponds to, or number of %s's.
// Messing up will result in garbage data being read, and potentially with some luck, the BEL sound being logged! ;)
void MungPlex::Log::LogInformation(MungPlex::LogMessages::LogMessageIntegers _Enum, ...) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead, consider implementing a logging library such as spdlog(vcpkg install spdlog)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to add another library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not? It's a relatively small library. I would add it, good logging is always important for a well-developed app 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright! I’ll add it I guess

}
}

void MungPlex::WebsocketClient::SendBinaryData(std::vector<uint8_t> _buffer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's likely nicer and cleaner to exchange JSON files instead of buffers. In the long term you'll probably be better off, especially for debugging purposes. nlohmann-json is already part of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I choose to use buffers to save on network resources on LawnMeowers end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it really make such a difference? You could apply zlib compression on top to save network resources as well 😅
Boost zlib compression example: https://stackoverflow.com/a/65911734

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll switch them all to json too I guess, thanks for all the feedback and I’ll update the code as soon as possible!

"DocumentsPath": "C:\\Users\\s_sch\\Documents",
"EnableRichPresence": true,
"Scale": 0.8941890001296997
"DocumentsPath": "C:\\Users\\Tim\\Documents",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this file shouldn't be tracked by git either if it contains personalized settings and file paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn’t realize this file was commuted, my bad

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.

2 participants