akmdm Thanks for the new patches! Quick question on 10719: Why are bots now informed about chat messages, even if they are rejected by internal cleaning? Same to the event handling: Why does the event handling gets handed over the uncleaned chat message? To make the review easier, it would also be good to split this change into two patches, one for the chat and one for the damage part.
akmdm Reply To torr_samaho
Thanks for the new patches! Quick question on 10719: Why are bots now informed about chat messages, even if they are rejected by internal cleaning? Same to the event handling: Why does the event handling gets handed over the uncleaned chat message? To make the review easier, it would also be good to split this change into two patches, one for the chat and one for the damage part.
Ah, I completely missed the rejection part, not a problem to change though. Would you also suggest that the cleaned ChatString now be sent into BOTCMD_SetLastChatString as opposed to the uncleaned pszString then? I think I'll divide this patch into three parts: one that adds the event handling code, another for the chat event, and another for the damage event.
Good point. I think the cleaned string should be handed to the bot code.
Regarding 10727: Can you elaborate why this fixes a crash? It sounds quite worrisome that the code block now guarded by an extra condition can cause a crash.
akmdm Reply To torr_samaho
Good point. I think the cleaned string should be handed to the bot code. Regarding 10727: Can you elaborate why this fixes a crash? It sounds quite worrisome that the code block now guarded by an extra condition can cause a crash.
The extra condition I added prevents HUD_BuildPointString from executing when the current game mode only supports PLAYERSEARNKILLS, since the function Team_GetKillCount doesn't exist. Therefore, the scoreFunction variable in HUD_BuildPointString would remain uninitialized and crash the application. In theory, HUD_BuildPointString could return an empty string instead but then there would be an empty line on the scoreboard. The only other place where HUD_BuildPointString may be called is when HUD_BuildPlaceString is called, which already blocks out PLAYERSEARNKILLS.
Adding a kill count for teams is also kind of a big endeavour that I don't really want to handle right now, if necessary, so I rather block off the call to the aforementioned function for now.
Not displaying this is completely fine. Since you know that HUD_BuildPointString will crash if called under certain conditions, I would additionally safeguard that function though to return an empty string as you suggested.
akmdm I uploaded the now updated 10719 (split into three patches) and 10727 patches. For the former, I also decided to remove colour codes in the saved chat messages used with GAMEEVENT_CHAT and what gets sent to the bots, as it doesn't seem important to preserve them for the events.
Thanks for the updates! Now that is 10719 is split, I noticed some more questions. The StaticStartTypedScripts call in GAMEMODE_HandleEvent may trigger multiple scripts, right? But we can only return one value, which seems to be the return value of the last triggered script. Is that desired? Are modders supposed not to create more than one script per event? Also, can you elaborate why the case SCRIPT_Running: part in RunScript is necessary? Isn't the result of the event supposed to be the result of the event script?
akmdm Reply To torr_samaho
The StaticStartTypedScripts call in GAMEMODE_HandleEvent may trigger multiple scripts, right? But we can only return one value, which seems to be the return value of the last triggered script. Is that desired? Are modders supposed not to create more than one script per event?
I had this in mind when I designed the event handling, and it's still okay if modders want to create multiple scripts that may handle the same event, as the result value of the event will be transferred from one script to the next. The only thing that they might not want to do, from an organization standpoint, is change the result of the event across multiple scripts, since the last triggered script would technically have the final say on this, and they might not exactly know the order in which all event scripts get triggered first. So as long as they change the result value in only one script, executing any additional event scripts won't affect the result of the event. What is your opinion on this?
Reply To torr_samaho
Also, can you elaborate why the case SCRIPT_Running: part in RunScript is necessary? Isn't the result of the event supposed to be the result of the event script?
This kind of goes along with the explanation above, where the result value of the event must be transferred from one event script to the next in case there are multiple event scripts, so the result of the event doesn't get messed up. In addition, this is necessary for GAMEEVENT_ACTOR_DAMAGED as the result value must be initialized to the original damage that the target actor was receiving. if the result value changes to something else, then that's the new damage the actor will take.
EDIT: There is one caveat with GAMEEVENT_ACTOR_DAMAGED because the event is triggered after the actor's armour had already absorbed some of the damage. The best solution is to create a variant of that event which gets called just before the armour absorbs the damage. Though to have to optimize another event, I might end up replacing all those DECORATE flags with a new actor property that can handle which events the actor triggers separately.
akmdm)
Thanks for the clarification! With that I now understand what the comment above case SCRIPT_Running:. Perhaps it would be helpful to increase that comment a bit to point out that the return values are transferred from one event script to the next with this.
akmdm Sounds like a good idea, so I uploaded 10717_v3A (I accidentally put a typo in the name) and elaborated more on the comment in question.
Thanks! I added the updated patch. Regarding your comment on the caveat of GAMEEVENT_ACTOR_DAMAGED : Do you still want to change that 10719_B or shall I check it as it is now?
akmdm Reply To torr_samaho
Thanks! I added the updated patch. Regarding your comment on the caveat of GAMEEVENT_ACTOR_DAMAGED : Do you still want to change that 10719_B or shall I check it as it is now?
Feel free to check it as it is right now. I'll deal with the caveat once 10719_B has been pushed and everything is set into stone.
Ok, 10719_B looks fine and I added it.
Regarding 10719_C, can you elaborate the use case for this mechanism? I wonder why the chat event can prevent the chat message from being displayed, but not bots from being informed about it and also not prevent it from being saved.
akmdm Reply To torr_samaho
Ok, 10719_B looks fine and I added it. Regarding 10719_C, can you elaborate the use case for this mechanism? I wonder why the chat event can prevent the chat message from being displayed, but not bots from being informed about it and also not prevent it from being saved.
Some people brought up the idea of a proximity-based chat system where players only receive chat messages from players who are close enough to them. This sounds like an interesting concept that could be useful in mods like Who Dun It where players can't hear each other from across the map. I also noticed that modders have already been using GAMEEVENT_CHAT for command-based operations in ACS, so in some of those cases it might help if the command message didn't get printed on the screen.
The chat message still needs to be saved, whether or not it gets printed, since GAMEEVENT_CHAT might still want to check and/or interpret the message the player sent. I wasn't sure if I should prevent bots from being informed about messages that don't get printed, but thinking about it more they probably should only react to chat messages that get printed, much like a real player.
akmdm I amended 10719_C so that now, if GAMEEVENT_CHAT returns 0 and the chat message doesn't get printed, bots also won't be notified about the message.
akmdm Update on the Jump Maze regression bug we discussed during our last dev meeting: we couldn't find any GZDoom commits that would fix the regression, but DrinkyBird found the problem himself and sent a PR over to their repository, which already got pushed: https://github.com/coelckers/gzdoom/commit/bf1577a984928fa01c885a64fd283540a7d1d5c5
A quick test confirmed that the commit did indeed fix the message bug, so we should transplant it over to our repository.
Great! I transplanted and pushed the patch.
akmdm I'm going to start uploading patches earlier than usual. So far, nothing but just a couple of small regression fixes (10732, 10740, and 10742).
Thanks! Looking all good, so I pushed the new stack of patches.
akmdm I uploaded another small patch to fix an issue that has just come to my attention. 10744.patch fixes the losing player not being forced to spectators if the level changed while in the middle of a duel's win sequence. Testing by me and reporter showed that the issue seems to be fixed now.
10744 looks fine, it's in our repo now.
akmdm akmdm I added one patch: 10745.patch, which changes the version string to 3.1 and BUILD_ID/BUILD_ID_STR to release, like standard procedure. I didn't add the ZA_3.1 tag yet, as I assume this should be done just before pushing any commits post-3.1.
akmdm)
akmdm akmdm I decided to forcibly disable sv_smoothplayers in the release build for the time being. Further testing showed that there are still major problems with the skip correction and it doesn't seem like much can be done about them at this time. I'd feel more comfortable making the necessary fixes/improvements with the system for a later release (i.e. 3.1.1). Fortunately, since the system is entirely server-sided, doing any future testing shouldn't be too difficult.
So now, there's two patches: 10745.patch which makes sv_smoothplayers not present in the release build, and 10746.patch which updates the version string and BUILD_ID/BUILD_ID_STR.
akmdm 
Fork
A continuation of patches for 3.1.