akmdm akmdm akmdm First, thanks a lot for the patches! It's great to see that you already took care of a bunch of different issues. I'll review one patch after another and will leave comments in here. Side note: You could make the reviewing slightly easier, if you submit each patch separately.
The BFG obituary patch (3231) looks good, there is just one minor style aspect that should be changed: Whenever we make changes in (G)ZDoom code, we leave a comment with the initials of the author of the change to point out that something was changed in (G)ZDoom code. This is necessary to facilitate porting changes from GZDoom. This way, one can distinguish if a change comes from us or from an out of sequence GZDoom backport.
EDIT: The same applies to the obituary change in p_interaction.cpp.
EDIT2: The same applies to the OpenAL removal from the menu. Please just comment out the corresponding line and precede it with a comment so we know that we removed it from ZDoom's code.
akmdm Thanks for your response. I'll remember to add comments whenever I make changes to (G)ZDoom's code in the future. I'll also send the revisions as separate patches next time to make reviewing easier.
Did you want me to resend all the revisions from this changeset, or just a new patch that adds the comments wherever necessary?
akmdm)
Resending just those that need changes is fine. I'm not fully done with checking all of them.
Regarding 3779: This seems to be a regression of 9479c4fa2f10ca1cf39ff93ee59da8bf57d64f54. It would be good to check with Leonard whether your patch will interfere with something he had in mind with his change.
Nice to see all these patches!
One nitpicky thing which IMHO should be changed is the patch with the additional gamemode acs functions: the code which handles them could be moved to gamemode.cpp into functions which could be called in p_acs.cpp, so that there would be no need to include the specific gamemodes headers. Do you think it would be possible?
akmdm Absolutely! This morning while I was gone, I actually wanted to move that part of the code over to game mode.cpp so that the additional headers didn't need to be included anymore and the function wouldn't look so messy. I can certainly change this.
The patch for 3126 and 3149 looks fine. I pushed it to our main repo.
akmdm akmdm akmdm I reattached all the patches that needed to be revised, these should address the changes that needed to be made. Thank you guys for your suggestions.
I'll talk to Leonard about that other ticket (3779) and find out if the patch interferes with anything he changed.
akmdm)
akmdm akmdm akmdm akmdm akmdm akmdm akmdm In 11339.patch, when I edited client_CreateTranslation(), I used a somewhat hacky method to determine whether or not the translation sent by the server was de-saturated or not instead of adding another variable in the SERVERCOMMANDS themselves by checking the size of the byte stream that was left. In theory, the byte stream for de-saturated translations is bigger than for RGB translations because they require floats to be sent as opposed to bytes. I hope this hack is okay.
Thanks for the updates! I'll look through all of them. Here is what I have so far: Patches 11330 and 11331 look fine, I pushed them to our main repo. Regarding 11332: In the implementation of ACSF_SetCurrentGamemode in the block starting with "// AK If we're changing to duel, make sure there's not too many players either.", you should use GAME_CountActivePlayers instead manually looping trough all the player slots to count the active ones.
EDIT: I added 11333 as well.
EDIT2: 11334 looks fine as well, since the function number depends on the functions from 11332 to be added, I'll wait with adding 11334 till 11332 is in.
EDIT3: I added 11335 as well.
akmdm Hi Torr, thanks for the review. I'll replace that loop with the function you mentioned in 11332 when I come home tonight.
Sounds good! Meanwhile, I also added 11338.
Regarding 11339: I fear your hack is not going to work reliably. pbStreamEnd indicates the end of the packet, not the end of the current command. So, this will only work if SVC_CREATETRANSLATION2 is the last command in the current packet.
akmdm Sounds good, I'll fix 11339 too. By the way, what's the status on 11336 and 11337?
Why is there a map reset parameter to SetCurrentGamemode? I think that it would be better to just call SetCurrentGamemode, followed by a map change call to do the same thing.
akmdm I added the map reset parameter because it would've been pointless to execute the code that add/remove the active players from teams if necessary, then transfers the state and countdown time of the old gamemode to the new one, if the map is going to be reset immediately afterwards due to a ChangeLevel() call. It's also meant to make it more convenient for the modder, since more likely than not they'll need to reset the map to fix the item spawns. They could do this in one single function, call, rather than two.
The reset parameter was intended to be optional.
EDIT: One more question, do you still have regular developer meetings anywhere, like on IRC? If so, it might be easier if I joined in so we can further discuss these issues, and any future issues on the tracker. I understand if time might be an issue and this isn't possible right now.
akmdm)
Reply To akmdm
I added the map reset parameter because it would've been pointless to execute the code that add/remove the active players from teams if necessary, then transfers the state and countdown time of the old gamemode to the new one, if the map is going to be reset immediately afterwards due to a ChangeLevel() call. It's also meant to make it more convenient for the modder, since more likely than not they'll need to reset the map to fix the item spawns. They could do this in one single function, call, rather than two. The reset parameter was intended to be optional. EDIT: One more question, do you still have regular developer meetings anywhere, like on IRC? If so, it might be easier if I joined in so we can further discuss these issues, and any future issues on the tracker. I understand if time might be an issue and this isn't possible right now.
EDIT2: On second thought, I decided to remove the map reset parameter from SetCurrentGamemode() after all, so now it'll only accept one parameter: the new game mode as a string.
akmdm Here are the updated patches to 11332 and 11339 Torr, based on the suggestions you mentioned before.
akmdm I uploaded some more patches for review. I wanted to ask if there's any more progress on fixing ticket #3787 (delays in certain player actions online). This seems to be the one issue that's holding back any further testing of 3.1. If not, then I might try to see if there's anything I can do to fix/improve the movement regulator as suggested.
I added patches 11332_r2, 11334, 11339_r2, 11340 and 11341. I still have to take a closer look at 11336, 11337 and 11343.
11340 requires some discussion to see what the intended behavior should be.
And yes, we still have regular dev meetings on IRC. We meet on Sundays at 9pm CET in #zadev.
EDIT: I also added 11337.
EDIT2:
I uploaded some more patches for review. I wanted to ask if there's any more progress on fixing ticket #3787 (delays in certain player actions online). This seems to be the one issue that's holding back any further testing of 3.1. If not, then I might try to see if there's anything I can do to fix/improve the movement regulator as suggested.
Right now, none of us has time to really work on this issue. If you'd like to give this a try, please go ahead. Would be great if this is fixed. I was already considering to simply restore the old 3.0 behavior as a stop gap solution, but a proper fix would be much better.
akmdm Reply To torr_samaho
Right now, none of us has time to really work on this issue. If you'd like to give this a try, please go ahead. Would be great if this is fixed. I was already considering to simply restore the old 3.0 behavior as a stop gap solution, but a proper fix would be much better.
I understand, I'll do my best to fix this issue. Thanks for letting me know about the time and place of the developer meetings, I wanted to ask if I could have access to this channel. If so, I can try attending meetings whenever possible. Thank you for your review today as well, take care!
akmdm)
akmdm Sorry, I couldn't get a lot done this week, so I submitted whatever I have ready now. I'm still working on improving the movement regulator and perhaps fixing other network issues too.
No need to be sorry. I'm very happy that you are contributing patches, don't feel rushed!
I added 11345. Since 11344 depends on 11343, I have to finish checking 11343 first.
11342 seems to fix ACS HUD messages, but from what I can tell it unintentionally changes what happens for internal HUD messages. 0xff000000 should only be added to ids of HUD messages created by ACS. SERVERCOMMANDS_PrintHUDMessage* is not only called by ACS, so its behavior shouldn't be generally changed. I'd try to instead pass "id ? 0xff000000|id : 0" (better save that as a variable and pass that) as id to the SERVERCOMMANDS_PrintHUDMessage* calls inside p_acs.cpp instead of unconditionally changing the client side behavior of these functions.
Can you elaborate your idea for 11343 and 11344? So far it seems to me that the code is more complicated than it should be, but I may be overlooking something.
akmdm Reply To torr_samaho
11342 seems to fix ACS HUD messages, but from what I can tell it unintentionally changes what happens for internal HUD messages. 0xff000000 should only be added to ids of HUD messages created by ACS. SERVERCOMMANDS_PrintHUDMessage* is not only called by ACS, so its behavior shouldn't be generally changed. I'd try to instead pass "id ? 0xff000000|id : 0" (better save that as a variable and pass that) as id to the SERVERCOMMANDS_PrintHUDMessage* calls inside p_acs.cpp instead of unconditionally changing the client side behavior of these functions.
Ah I see. That's something I overlooked, my mistake. I'll go make a revision for this patch and do what you suggested.
Reply To torr_samaho
Can you elaborate your idea for 11343 and 11344? So far it seems to me that the code is more complicated than it should be, but I may be overlooking something.
I'll try to summarize my idea for these two patches:
- I found that on the server, S_IsActorLoopingSound() always returns false because the server never executes any code in S_StartSound(). The server will only pass a command to tell the clients to play the sound.
- I added an array list that contains a list of all the sound channels of actors currently playing a looping sound on the server. New entries may be added to the list, while existing entries may be either replaced (if the sound channel loops a different sound) or removed if the channel isn't looping anymore. Other things like channel flags, volume, and attenuation must be remembered too.
- This list is used by the server to synchronize any looping sounds with newly connected clients.
- When a serversided actor is going to play a looping sound, the server will first check if this actor is already looping the same sound on the same channel. If so, it does nothing.
- If the sound isn't already looping on the specified channel, the server will update the array list accordingly and send a SERVERCOMMAND to tell the clients to play the sound on the actor.
- In patch 11344, I modified the StopSound() ACS function to have the server also inform the clients to stop the sound on the actor's channel too. Before, the server would call this function but the changes wouldn't be updated on the client's end. This required having to define a new SERVERCOMMAND, since one didn't exist before.
I'll admit, the implementation is somewhat complicated, especially where it checks if the actor is already looping the sound on the channel. However, this is to prevent the server from needlessly sending out another command to the clients to replay the looping sound (both the CHAN_LOOP flag and "looping" parameter must be accounted for) if it isn't going to change anything, meaning it's consuming bandwidth for nothing. My changes make it so the server only sends the command if the sound isn't already looping on that channel.
A little side note, I want to ask if it's necessary for the DECORATE functions A_PlaySound() and A_PlaySoundEx() to need the server to send a command to the clients to play the sound. A_StopSound() doesn't have any netcode but still works since the server and clients are executing the actor's code asynchronously.
I think we can do the same with these two functions as well, as this will reduce the complexity of the two patches and optimize net traffic. The ACS version of these functions still require the commands to be called on the server in case they're called from a serversided script.
akmdm)
akmdm Hi again, I added two new patches. For 11342, I also want to address ticket #3623 since the newer features of the ACS HUD messages (alpha/blending, layer and visibility flags, clipping rectangle, and wrap width) introduced in 3.0 don't work if the message is printed on the server's end. This will require a little more editing for them to work, but I think I'll submit everything under one single patch.
For 11343 and 11344, I want to first try and simplify the code I added so it's less complicated before resubmitting it. I'm still unsure on omitting the netcode from A_PlaySound and A_PlaySoundEx, but I want to hear what you think first before I do anything.
EDIT: Torr, I sent you a PM on the forums. If you have time, could you read it please? Thanks again.
akmdm)
A little side note, I want to ask if it's necessary for the DECORATE functions A_PlaySound() and A_PlaySoundEx() to need the server to send a command to the clients to play the sound.
Only the server is guaranteed to know the correct arguments A_PlaySound() and A_PlaySoundEx() is called with, since the arguments can be results of function calls. This is why the server needs to handle this.
A_StopSound() doesn't have any netcode but still works since the server and clients are executing the actor's code asynchronously.
This is mainly a bandaid since the server doesn't actually play the sounds so it doesn't know what needs to be stopped.
I added your two new patches and will check your PM.
akmdm Reply To torr_samaho
Only the server is guaranteed to know the correct arguments A_PlaySound() and A_PlaySoundEx() is called with, since the arguments can be results of function calls. This is why the server needs to handle this.
I understand now, that makes sense then. Thanks for mentioning this. I resubmitted the patches that fix the looping sounds and the StopSound ACS function (looping_sounds.patch and stop_sound.patch respectively). I tried simplifying the former as much as possible, so it shouldn't be too complicated anymore.
EDIT: In setdeadspectator_obituary.patch the simplest way I could think of removing the obituary was by adding a new damage type that is used in SetDeadSpectator then modify ClientObituary to not do anything if this is the MoD of the player. Hopefully this is still okay, considering modders could also use this in ACS functions that accept the damagetype as a string.
akmdm)
Thanks for the steady stream of patches! Greatly appreciated :).
I added translation_update.patch, player_setteam.patch and acs_playerscore.patch. Unfortunately, I cannot apply looping_sounds.patch and stop_sound.patch without much tinkering with the patch file. Can you do me a favor and resubmit them based on the current head of our main repository?
akmdm Reply To torr_samaho
Thanks for the steady stream of patches! Greatly appreciated :). I added translation_update.patch, player_setteam.patch and acs_playerscore.patch. Unfortunately, I cannot apply looping_sounds.patch and stop_sound.patch without much tinkering with the patch file. Can you do me a favor and resubmit them based on the current head of our main repository?
My pleasure! I'll also resubmit those two patches as soon as I can.
EDIT: Torr, I resubmitted those two patches. You should be able to add them without any more conflicts.
akmdm)
akmdm Thanks for the updated patches! I could apply looping_sounds_r2.patch without problems and started looking at it in detail. Here is what I noticed so far:
- Are you sure that SERVER_ClearLoopingChannels is working? Wont this get stuck in the while loop as soon as "g_LoopingChannelListi.Actor != pActor"? The index i is not incremented if that is false.
- A_PlaySound and ACSF_PlayActorSound seem to handle the same logic quite differently: (Unfortunately, I didn't manage to keep the formatting of the code)
// AK If we're the server, we have to update the list of looping sound channels. if ( NETWORK_GetState( ) == NETSTATE_SERVER ) { // AK Don't loop this sound if it's already looping on this channel. if (( channel & CHAN_LOOP ) && ( SERVER_IsChannelLooping( self, channel&7, soundid ) )) return;
SERVER_UpdateLoopingChannels( self, channel, soundid, volume, attenuation, false ); }
S_Sound (self, channel, soundid, volume, attenuation, true ); // BC Inform the clients.
vs.
// AK Check if this actor isn't already looping the same sound on the server. if (( NETWORK_GetState() != NETSTATE_SERVER ) || (( chan & CHAN_LOOP ) && !SERVER_IsChannelLooping( spot, chan & 7, sid ))) { S_Sound(spot, chan, sid, vol, atten, true); // EP Inform the clients.
// AK Update the server's list of looping sound channels. if ( NETWORK_GetState() == NETSTATE_SERVER ) SERVER_UpdateLoopingChannels( spot, chan, sid, vol, atten, false ); }
I'd prefer the A_PlaySound way, since it only adds one delta.
EDIT: I added armor_clearinventory.patch.
akmdm akmdm I submitted another revision to the looping sounds issue: looping_sounds_r3.patch. This fixes the loop in SERVER_ClearLoopingChannels and restructures the changes in ACSF_PlaySound so that they're similar to the changes in A_PlaySound. stop_sound_r2.patch should be importable without any issues too.
EDIT: Also added a new patch for a change I made a while ago. This fixes the color codes being ignored in obituary messages defined within an actor's class. The change is now inside ClientObituary and shouldn't interfere with the changes Leonard made in past commits.
EDIT 2: Added another patch that fixes some issues in how the new lagging scoreboard icon should appear, brought up by DJB.
akmdm)
Thanks for the updates! I just added looping_sounds_r3.patch, 10231.patch and 10232.patch. Is anything still missing? This ticket is quite convoluted by now.
akmdm Reply To torr_samaho
Thanks for the updates! I just added looping_sounds_r3.patch, 10231.patch and 10232.patch. Is anything still missing? This ticket is quite convoluted by now.
stop_sound_r2.patch still needs to be added, then it will be complete.
I agree, I think we should close this ticket going forward from here.
EDIT: Sorry if I keep asking this, but is there any conclusion on the acceptance of 11336.patch (the SBARINFO command)? I can open a separate ticket for this so we can finally close this one.
I'm planning to fix ACS HUD messages too by the end of this week, I'll create a separate ticket for that as well.
EDIT 2: I haven't gotten the ACS HUD messages fixed yet unfortunately. I will try to finish this for next week.
akmdm)
stop_sound_r2.patch looks good except for one thing. I think the implementation of SERVERCOMMANDS_StopSound should be // AK The actor must have a NetID so we can tell clients to stop the sound on their channel. if ( EnsureActorHasNetID (pActor) ) return; ServerCommands::StopSound command; command.SetActor( pActor ); command.SetChannel( lChannel ); command.sendCommandToClients( ulPlayerExtra, flags ); (can't get the formatting to work. but I hope you can still see what I mean).
EDIT:
EDIT: Sorry if I keep asking this, but is there any conclusion on the acceptance of 11336.patch (the SBARINFO command)? I can open a separate ticket for this so we can finally close this one.
No worries. Thanks for reminding me! I simply overlooked that one (too many different things in here ;)). Can you create a separate ticket for this and also include a minimal example that uses the new SBARINFO command?
EDIT 2: I haven't gotten the ACS HUD messages fixed yet unfortunately. I will try to finish this for next week.
No problem, take your time.
akmdm Thanks for your review Torr! Don't worry, I was able to see the code you posted (OSDN's comment formatting isn't that great). I attached a new revision: stop_round_r3.patch based on the changes you mentioned.
No worries. Thanks for reminding me! I simply overlooked that one (too many different things in here ;)). Can you create a separate ticket for this and also include a minimal example that uses the new SBARINFO command?
No problem, I created a new ticket for this feature including the patch and minimal example wad. From here on I'll just create new tickets for future sets of patches so that everything is organized and easier to find. Thanks again. :)
Thanks for the updated StopSound patch. I just added it. We have taken care of all open issues in this ticket now, right?
akmdm Reply To torr_samaho
Thanks for the updated StopSound patch. I just added it. We have taken care of all open issues in this ticket now, right?
Looks like all the issues on this ticket have been resolved, so we may now close this. Thanks again!
You're welcome! I'm closing this then.
Fork
This patch changeset contains several commits addressing the following issues as reported on the tracker: 3231, 3126, 3149, 3313, 3850, 3783, and 3779.