PCD_DROP changes script result value as a side effect

[ZzZombo]

https://github.com/rheit/zdoom/pull/392.

I found out the reason today why the hell all of my scripts randomly stop working. They all do not contain any call to SetResultValue(). But their result value is used in the calling routine, if it's false the routine ends. So I was quite surprised when the said routine stops working half the time instead of working all the time. I first blamed BCC, then checked the ACS VM code and found out PCD_DROP, that is used to decrement the stack pointer in Hexen, in ZDoom falls through to the next case label, that is PCD_SETRESULTVALUE... Since it's used to unwind the stack if the return value of a function wasn't used, and most of the time it's void return type functions, it ultimately sets the script result value to 0 that breaks the convention of implicit 1 result value unless changed by SetResultvalue().

Blzut3 pointed out some possible cases where people unintentionally called a script expecting it to return 0 without actually calling SetResultValue(0), but PCD_DROP did that and they thought it works as desired, but I don't think those cases reach enough size to consider backwards compatibility, so I made the pull request. However second opinion on this is welcome. There also were some ways to fix this w/o changing the VM, but they unfortunately require compiler support, and Positron for one refused to add that.

[Blzut3]

Additional context from Zandronum IRC: The change was made in 2.0.90 (on April 20th, 2004). Presumably while working on strifehelp.acs the original plan was probably to pass the result of action special calls as the result of a script call. Randi presumably found a need for finer control and introduced SetResultValue without reverting the change to PCD_DROP. I did document this quirk when documenting the VM, but didn't make anything of it then. Given how few people seem to have notice, my assumption is that the chance for compatibility issues by changing PCD_DROP now is higher than leaving it be. As far as vanilla hexen goes, I think the worst that can happen is a switch doesn't animate and even that is unlikely (not many hexen pcodes return a value that would need to be routinely dropped).

My suggestion was for compilers to allocate a script variable and use PCD_ASSIGNSCRIPTVAR in place of PCD_DROP which will work without requiring any changes in the VM (and provided no script variable count pressure will even work in vanilla Hexen).

Another alternative to outright fixing PCD_DROP would be to introduce a new script chunk for compatibility which allows compilers to opt into "new" behavior. In this case the flag could also be set for ACS0 modules.

[Graf Zahl]

I think it should still be fixed, and if problems come up, a compatibility option should be introduced. It wouldn't surprise me if there's also some maps out thee that subtly misbehave due to this. If some map implicitly relies on this it should be considered undefined behavior.

[randi]

PCD_DROP has always been solely for popping the topmost element off of the stack and throwing it away. It has never had any other purpose.