-
Notifications
You must be signed in to change notification settings - Fork 31
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
integration of Chocolate Doom network code #280
Conversation
|
||
// Call D_QuitNetGame on exit: | ||
|
||
//I_AtExit(D_QuitNetGame, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the server cannot be properly closed with the standard atexit()
. Should we implement I_AtExit()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we implement I_AtExit()?
We probably should (using the implementation in Pr+). But that's orthogonal to this PR. Let's not mix things up too much.
I would like this to be reviewed.
|
This only applies to network demos? For singleplayer demos it still works, right?
This is expected, as we only add the minimal possible amount of libtextscreen.
This is inherited from Crispy, I guess. fabiangreffrath/crispy-doom#747
This is probably the same as here: |
@@ -2230,7 +2243,7 @@ void D_DoomMain(void) | |||
{ | |||
I_StartTic (); | |||
D_ProcessEvents (); | |||
G_BuildTiccmd (&netcmds[consoleplayer][maketic%BACKUPTICS]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the [maketic%BACKUPTICS]
part and why was it dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
netcmds
declaration is different in MBF, I replace it with Choco version.
@@ -1,580 +1,270 @@ | |||
// Emacs style mode select -*- C++ -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the pristine d_net.c
file from Choco?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but without the support of other games (heretic/hexen/strife)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I added sync of Boom/MBF options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is also where lowres_turn
hides...
@@ -49,6 +49,18 @@ set(WOOF_SOURCES | |||
m_swap.h | |||
midifile.c midifile.h | |||
mmus2mid.c mmus2mid.h | |||
net_client.c net_client.h |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all the other net_*
files taken pristine from Choco or are there any changes that need to get documented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only changes are for other games and fixing gcc/clang warnings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I commented out "SecureDemo" experiment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never heard of that. 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at NET_StartSecureDemo
in net_query.c
Not sure how it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to be actually used anywhere.
It doesn't work in all types of demos due to the |
|
||
if (net_player_name == NULL) | ||
{ | ||
net_player_name = NET_GetRandomPetName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some odd reason, I have my problems with this "pet name" thing. I guess it's too much of an Choco inside joke to apply here. Could you replace this initialization with a GetUserName()
on WIN32 and getlogin()
elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to name them "Player 1" ... "Player 4"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Na, that's a bit lame, isn't it? 😉 I think even Vanilla had the sprite colors as player names, i.e. "green", "brown", "indigo", "red".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should invent dogs related joke here 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, that's nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge it? |
Yes, I think we can. Everything works except fastdemo. |
Yeeha! Fastdemo will require a separate fix, but it was more important to finally get this networking code merged before. |
I think this is a much cleaner attempt. Original code changes are kept at a minimum, take a look if you have time. Connects successfully to Crispy Doom with
complevel vanilla
. Also fixes #279.