Ticket #40871

Various changes/fixes for tickets reported on tracker

Open Date: 2020-10-20 09:29 Last Update: 2020-11-23 05:52

Reporter:
Owner:
(None)
Type:
Status:
Open
Component:
(None)
MileStone:
(None)
Priority:
5 - Medium
Severity:
5 - Medium
Resolution:
None
File:
22

Details

This patch changeset contains several commits addressing the following issues as reported on the tracker: 3231, 3126, 3149, 3313, 3850, 3783, and 3779.

Ticket History (3/59 Histories)

2020-10-20 09:29 Updated by: akmdm
  • New Ticket "Various changes/fixes for tickets reported on tracker" created
2020-10-20 09:29 Updated by: akmdm
  • File kaminsky.patch (File ID: 5695) is attached
2020-10-25 13:04 Updated by: akmdm
  • File kaminsky.patch (File ID: 5695) is deleted
2020-10-26 05:22 Updated by: torr_samaho
Comment

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.

(Edited, 2020-10-26 05:35 Updated by: torr_samaho)
2020-10-26 05:39 Updated by: akmdm
Comment

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?

(Edited, 2020-10-26 05:45 Updated by: akmdm)
2020-10-26 05:48 Updated by: torr_samaho
Comment

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.

2020-10-26 05:52 Updated by: edward-san
Comment

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?

2020-10-26 05:58 Updated by: akmdm
Comment

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.

2020-10-26 06:13 Updated by: torr_samaho
Comment

The patch for 3126 and 3149 looks fine. I pushed it to our main repo.

2020-10-26 10:44 Updated by: akmdm
  • File 11332.patch (File ID: 5699) is attached
2020-10-26 10:45 Updated by: akmdm
  • File 11333.patch (File ID: 5700) is attached
2020-10-26 10:50 Updated by: akmdm
Comment

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.

(Edited, 2020-10-26 11:28 Updated by: akmdm)
2020-10-27 10:05 Updated by: akmdm
  • File 11332.patch (File ID: 5699) is deleted
2020-10-27 10:05 Updated by: akmdm
  • File 11333.patch (File ID: 5700) is deleted
2020-10-30 14:58 Updated by: akmdm
  • File 11335.patch (File ID: 5704) is attached
2020-10-30 14:59 Updated by: akmdm
  • File 11336.patch (File ID: 5705) is attached
2020-10-30 22:38 Updated by: akmdm
  • File 11335.patch (File ID: 5704) is deleted
2020-10-30 22:38 Updated by: akmdm
  • File 11336.patch (File ID: 5705) is deleted
2020-11-01 14:59 Updated by: akmdm
Comment

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.

2020-11-02 05:36 Updated by: torr_samaho
Comment

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.

(Edited, 2020-11-02 05:59 Updated by: torr_samaho)
2020-11-02 06:14 Updated by: akmdm
Comment

Hi Torr, thanks for the review. I'll replace that loop with the function you mentioned in 11332 when I come home tonight.

2020-11-02 07:00 Updated by: torr_samaho
Comment

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.

2020-11-02 07:15 Updated by: akmdm
Comment

Sounds good, I'll fix 11339 too. By the way, what's the status on 11336 and 11337?

2020-11-02 07:30 Updated by: tespi
Comment

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.

2020-11-02 08:16 Updated by: akmdm
Comment

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.

(Edited, 2020-11-02 08:24 Updated by: akmdm)
2020-11-03 13:20 Updated by: None
Comment

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.

2020-11-03 14:59 Updated by: akmdm
Comment

Here are the updated patches to 11332 and 11339 Torr, based on the suggestions you mentioned before.

2020-11-08 14:04 Updated by: akmdm
Comment

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.

2020-11-09 05:52 Updated by: torr_samaho
Comment

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.

(Edited, 2020-11-09 06:29 Updated by: torr_samaho)
2020-11-09 06:45 Updated by: akmdm
Comment

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!

(Edited, 2020-11-09 10:28 Updated by: akmdm)
2020-11-15 22:42 Updated by: akmdm
Comment

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.

2020-11-16 01:17 Updated by: torr_samaho
Comment

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.

2020-11-16 05:43 Updated by: torr_samaho
Comment

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.

2020-11-16 05:51 Updated by: torr_samaho
Comment

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.

2020-11-16 07:11 Updated by: akmdm
Comment

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.

(Edited, 2020-11-16 09:50 Updated by: akmdm)
2020-11-22 22:39 Updated by: akmdm
Comment

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.

(Edited, 2020-11-23 04:21 Updated by: akmdm)
2020-11-23 05:52 Updated by: torr_samaho
Comment

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.

Attachment File List

  • kaminsky_r2.patch(21KB)
  • 11330.patch(1KB)
    • BFG Tracer Obituary
  • 11331.patch(2KB)
    • Added missing gameplay/compatibility options, and removed obsolete option in MENUDEF
  • 11332.patch(11KB)
    • New ACS functions: SetGamemodeLimit(), SetCurrentGamemode(), and GetCurrentGamemode()
  • 11333.patch(4KB)
    • New server variable: "sv_noobituaries"
  • 11334.patch(3KB)
    • New ACS function: SetPlayerClass() [3556]
  • 11335.patch(4KB)
    • Added new command "SetThingSpecies" to network protocol, fixes SetActorProperty with APROP_SPECIES not syncing to clients [3412]
  • 11336.patch(4KB)
    • New SBARINFO command: IfSpectator [not] [dead]
  • 11337.patch(2KB)
    • Added a new scoreboard icon that displays if a player is lagging to the server [3031]
  • lagmini.png(260bytes)
    • The scoreboard icon itself
  • 11338.patch(3KB)
    • Added new compatibility flag "compat_resetglobalvarsonmapreset" to reset all global ACS variables upon resetting the map like in survival [2435]
  • 11339.patch(11KB)
    • Fixed desaturated translations created with CreateTranslation() not syncing with clients in an online game [3438]
  • 11332_r2.patch(11KB)
  • 11339_r2.patch(12KB)
  • 11340.patch(2KB)
    • New ACS functions: SetPlayerChasecam() and GetPlayerChasecam() [1688]
  • 11341.patch(4KB)
    • gl_lights_size CVAR forced to default (1.0) with sv_forcevideodefaults [3202]
  • 11342.patch(2KB)
    • Serverside and clientside HudMessages cannot replace each other [3200]
  • 11343.patch(15KB)
    • Sync looping actor sounds with newly connected clients, and prevent redundant net traffic due to playing looped sounds [3181]
  • 11344.patch(5KB)
    • Added new extended command "StopSound" to network protocol, fixes StopSound() not working in online games
  • 11345.patch(3KB)
    • Fixed the strange warning text that appears when the client gains RCON access [3465]
  • stepmissile_fix.patch(1KB)
    • Fixed missiles with the STEPMISSILE flag disappearing in online games [3705]
  • setactorproperty.patch(9KB)
    • Added network optimizations in DoSetActorProperty [1609]

Edit

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Login