MantisBT - Zandronum
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0001858Zandronum[All Projects] Suggestionpublic2014-06-25 10:172018-09-30 21:35
ReporterDusk 
Assigned ToDusk 
PrioritynormalSeverityminorReproducibilityN/A
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target Version1.3Fixed in Version1.3 
Summary0001858: Client-to-server mechanism to ACS_Execute to supersede ConsoleCommand("puke")
DescriptionConsoleCommand is evil as we all know. One current 'valid' use of it is to make a puke request to call server-side ACS from the client-side.

I'd want to natively implement this behavior by adding a new rule to script calling: if a server-side NET script is being called from the client-side, the client would send a puke client command.

Right now, in such a case the client... runs the script on its own, causing a ton of desyncs since server-side scripts are the ones that alter the gamesim... I'd want to write this off officially as undefined behavior (it certainly isn't defined behavior in Zandronum source!) and replace the behavior with a client-to-server mechanism.

There's a few quirks to this behavior as a result of this being a client-to-server puke call:
- it only works with NET scripts. It would obviously be disastrous to let this work with non-NET scripts as this is client-to-server and thus it would let any client activate any script.
- the script activator will always be the local consoleplayer no matter what was the activator of the client-side script. Same reasoning as above - modified clients could set the activator at will otherwise and for instance cause other players to sell their weapons in AOW.
- being client-to-server, the packet sent is unreliable and there is no guarantee that the packet will ever reach the server.

These quirks are all also present with the current ConsoleCommand("puke") workaround.
Steps To Reproduce
Additional Information
TagsNo tags attached.
Relationships
child of 0001070new Dusk Phasing out the dangerous ConsoleCommand for various additional/improved ACS commands 
Attached Fileszip acs-client-to-server-test.wad.zip (675) 2014-06-29 16:52
/tracker/file_download.php?file_id=1238&type=bug
? test_serversidenonnetscriptfromclient.pk3 (1,015) 2014-07-09 20:39
/tracker/file_download.php?file_id=1249&type=bug
? ammocountdesync.pk3 (1,084) 2014-07-10 13:17
/tracker/file_download.php?file_id=1251&type=bug
png Screenshot_Doom_20140710_161537.png (98,433) 2014-07-10 13:18
/tracker/file_download.php?file_id=1252&type=bug
png
Issue History
Date ModifiedUsernameFieldChange
2014-06-25 10:17DuskNew Issue
2014-06-25 10:17DuskStatusnew => assigned
2014-06-25 10:17DuskAssigned To => Dusk
2014-06-25 10:18DuskRelationship addedchild of 0001070
2014-06-25 10:22DuskSummaryClient-to-server mechanism to ACS_Execute to supercede ConsoleCommand("puke") => Client-to-server mechanism to ACS_Execute to supersede ConsoleCommand("puke")
2014-06-25 12:27DuskNote Added: 0009739
2014-06-25 12:27DuskStatusassigned => needs review
2014-06-26 18:46Torr SamahoNote Added: 0009755
2014-06-29 10:53Torr SamahoNote Added: 0009779
2014-06-29 11:11Torr SamahoTarget Version => 1.3
2014-06-29 11:12Torr SamahoStatusneeds review => needs testing
2014-06-29 12:54DuskNote Added: 0009782
2014-06-29 12:54DuskStatusneeds testing => assigned
2014-06-29 16:52DuskFile Added: acs-client-to-server-test.wad.zip
2014-06-29 17:09DuskNote Added: 0009789
2014-06-29 17:09DuskStatusassigned => needs review
2014-06-29 19:29Torr SamahoNote Added: 0009792
2014-06-29 19:29Torr SamahoStatusneeds review => needs testing
2014-07-09 20:39QentFile Added: test_serversidenonnetscriptfromclient.pk3
2014-07-09 20:43QentNote Added: 0009924
2014-07-09 20:45QentNote Edited: 0009924bug_revision_view_page.php?bugnote_id=9924#r5306
2014-07-09 20:53QentNote Edited: 0009924bug_revision_view_page.php?bugnote_id=9924#r5309
2014-07-09 20:57QentNote Edited: 0009924bug_revision_view_page.php?bugnote_id=9924#r5310
2014-07-09 21:04DuskStatusneeds testing => assigned
2014-07-09 23:59QentNote Edited: 0009924bug_revision_view_page.php?bugnote_id=9924#r5313
2014-07-10 09:17ZzZomboNote Added: 0009929
2014-07-10 13:12DuskNote Added: 0009930
2014-07-10 13:12DuskNote Edited: 0009930bug_revision_view_page.php?bugnote_id=9930#r5317
2014-07-10 13:16DuskNote Edited: 0009930bug_revision_view_page.php?bugnote_id=9930#r5318
2014-07-10 13:17DuskNote Edited: 0009930bug_revision_view_page.php?bugnote_id=9930#r5319
2014-07-10 13:17DuskFile Added: ammocountdesync.pk3
2014-07-10 13:18DuskFile Added: Screenshot_Doom_20140710_161537.png
2014-07-10 13:19DuskNote Edited: 0009930bug_revision_view_page.php?bugnote_id=9930#r5320
2014-07-10 13:25DuskStatusassigned => needs review
2014-07-12 15:39Torr SamahoNote Added: 0009948
2014-07-12 15:39Torr SamahoStatusneeds review => feedback
2014-07-20 17:12Ijon TichyNote Added: 0010014
2014-07-20 17:14Ijon TichyNote Edited: 0010014bug_revision_view_page.php?bugnote_id=10014#r5372
2014-07-20 19:37DuskStatusfeedback => assigned
2014-07-28 12:54DuskStatusassigned => needs review
2014-07-28 13:30DuskNote Added: 0010063
2014-07-28 13:30DuskStatusneeds review => needs testing
2014-07-28 13:40DuskNote Edited: 0010063bug_revision_view_page.php?bugnote_id=10063#r5411
2014-07-28 13:40DuskNote Edited: 0010063bug_revision_view_page.php?bugnote_id=10063#r5412
2014-10-05 22:20DuskFixed in Version => 1.3
2014-10-05 22:22DuskNote Added: 0010344
2014-10-05 22:22DuskStatusneeds testing => resolved
2014-10-05 22:22DuskResolutionopen => fixed
2018-09-30 21:35Blzut3Statusresolved => closed

Notes
(0009739)
Dusk   
2014-06-25 12:27   
Here goes:'https://bitbucket.org/crimsondusk/zandronum-stable/commits/509c7f4f8a131a52e5086db754c18311f56991cb [^]'
(0009755)
Torr Samaho   
2014-06-26 18:46   
That sounds like a good idea. Will look at the implementation in detail.
(0009779)
Torr Samaho   
2014-06-29 10:53   
Looks good, transplanted. We just have to keep in mind that we'll need to introduce a compatflag to restore the old behavior in case there are mods that break because of this change.
(0009782)
Dusk   
2014-06-29 12:54   
This is undefined behavior we're addressing here... then again, all bugs kind of themselves already are that. I'll add a compat_clientacsexecute...
(0009789)
Dusk   
2014-06-29 17:09   
'https://bitbucket.org/Torr_Samaho/zandronum-stable/pull-request/55/added-new-compatflag [^]'
(0009792)
Torr Samaho   
2014-06-29 19:29   
Pulled.
(0009924)
Qent   
2014-07-09 20:43   
(edited on: 2014-07-09 23:59)
This causes strange behavior in Russian Overkill (using 2.0, 2014.07.09-20.19.28:'http://l.xd.cm:8080/misc/zanfiles/windows/x86/zandronum-2.0-windows-x86-2014.07.09-20.19.28.zip [^]'), shown in the example test_serversidenonnetscriptfromclient.pk3. Connect to a server, switch to NewChainsaw, and attack. With the compatflag off, you will get the error "P_CheckClientToServerACS: Cannot run server-side non-NET script 123 from the client" and it will give you the wrong number of cells (should give cells equal to the number of bullets you currently have).

(The WADs used on the server were
hr2final.wad - dad338e1968aea4df05d465e864d78f3
hr2-final-fix.wad - 0949cc1a6745ed477e34029f8d21d235
ro_pb_2.2_zan_01.pk3 - f51227da2b2c5375f5076195d41e46be
ro_class_v3_zan_01.wad - 7946173686660ef61e662ee117f689cc
newtextcolours1_170.pk3 - 242ee38839ed48fdb36bda4794208e8b

'http://www.mediafire.com/download/u0rucuzme7ugd1e/RO_pb_2.2_zan_01.pk3#! [^]' is a link to Russian Overkill. The gun in question was the SchwarzerZwerg2. The compatflag does work to restore old behavior, however.)
(0009929)
ZzZombo   
2014-07-10 09:17   
My mod seems to break from ^.
My weapon w/o the compat. flag just prints the error into client's console (why? isn't it the server what decides how actors should act?) and with it enabled just plainly skips all calls to ACS from its states, I even made the script in question to PrintBold() a meaningless message just to check if the game skips the ACS call, and it indeed does, even tho the whole animation plays as if nothing went wrong. So I'd increase the severity of this ticket, it completely breaks game.
(0009930)
Dusk   
2014-07-10 13:12   
(edited on: 2014-07-10 13:19)
What I think is going on is that both peers executed the ACS script on their own and acted accordingly with the results just happening to be consistent. Now the mechanism doesn't allow the script to be executed on the client's end (but does on the server!), the server never tells the client how much ammo was recieved and the ammo counts go out of sync.

EDIT: Indeed appears to be an unrelated bug that the mechanism has revealed. The new example wad ammocountdesync.pk3 has a chainsaw that will desync the cell ammo even if the compatflag is on.

In case anyone's wondering, the "rcon printinv 0" is a debug build-exclusive feature which allows the server to peek at other players' inventories.
(0009948)
Torr Samaho   
2014-07-12 15:39   
Hmm, ammocountdesync.pk3 reveals design problem of your new mechanism, in the sense that it changes behavior it was not supposed to change. Or did you intentionally want your mechanism also to cover DECORATE constructs like

     TNT1 A 10 A_GiveInventory("Cell", ACS_ExecuteWithResult(1, 0))

?

And you are right, this example also shows a Zandronum bug unrelated to your mechanism.
(0010014)
Ijon Tichy   
2014-07-20 17:12   
(edited on: 2014-07-20 17:14)
Why are we hijacking current syntax to achieve this, now? Since this sort of client -> server behaviour can't exist in ZDoom's network model, there's no reason to worry about ZDoom compatibility with this. An ACS_RequestExecute(snum[, a1[, a2[, a3]]]) (does not get blocked the way ACS_Execute does) would avoid compatibility issues and make the client->server exchange explicit, rather than implicit, which would avoid /many/ headaches.
(0010063)
Dusk   
2014-07-28 13:30   
(edited on: 2014-07-28 13:40)
The mechanism's been replaced with a new RequestScriptPuke ACS function now:

use the following to try it out:

special -122:RequestScriptPuke (1, 4);
(0010344)
Dusk   
2014-10-05 22:22   
Marking as resolved as 1.3 is now released.