Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return value when executing DoCommand from GS is the same as in TestMode #5561

Closed
DorpsGek opened this issue May 26, 2013 · 4 comments
Closed

Return value when executing DoCommand from GS is the same as in TestMode #5561

DorpsGek opened this issue May 26, 2013 · 4 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

Zuu opened the ticket and wrote:

In world gen, GSes can execute DoCommands without the GS being suspended. The purpose of this is to allow a GS to set up goals etc. before the game starts.

However, I found out that the return value of API calls made during world gen will be the same value as documented to be returned in TestMode (often zero).

The reason for this can be seen in script_object.cpp:301:

if (_generating_world) {
    IncreaseDoCommandCosts(res.GetCost());
    if (callback != NULL) callback(GetActiveInstance());
    return true;
} else if (_networking) {
    /* Suspend the script till the command is really executed. */
    throw Script_Suspend(-(int)GetDoCommandDelay(), callback);
} else {
    IncreaseDoCommandCosts(res.GetCost());

    /* Suspend the script player for 1+ ticks, so it simulates multiplayer. This
     \*  both avoids confusion when a developer launched his script in a
     \*  multiplayer game, but also gives time for the GUI and human player
     \*  to interact with the game. */
    throw Script_Suspend(GetDoCommandDelay(), callback);
}

In world generation, the method returns true. That the DoCommand succeds. However, that only otherwise happen in test mode. Thus all API methods will contain a return statement after the DoCommand call that is supposed to only get called in test mode which return 0 (or other constant value).

In ExecMode in non-world-gen, a Script_Suspend is thrown which takes the callback refrence. Higher up in the call hierarchy it will get handled.

In world gen, the callback is called before ScriptObject::DoCommand returns, which will set the correct return value. The problem is that ScriptObject::DoCommand then returns back to the API method which return a value (0) which is then set as the API method result value.

Reported version: trunk
Operating system: All

This issue was imported from FlySpray: https://bugs.openttd.org/task/5561
@DorpsGek
Copy link
Member Author

Zuu wrote:

With this patch I get a # 2 returned to the script. However I think this is a dead end. The problem is that when we call the callback, it will inject the result into the stack at a location further down in the stack intended for use when using suspend. So this solution need to update all callbacks so that they can either push to the stack for suspend or using the method in this patch. Whats good is that the number of callbacks is not very large, so codewise it is not a very large patch, but it complicate the callbacks further more.
http://devs.openttd.org/~zuu/sq1.patch

This second patch instead follow the same return technique as used in non-world gen Exec mode. It throws a suspension object but set suspend time to zero. At the receiving end, the code has been modified so that when suspend time is zero, the Squirrel VM is resumed again. Currently it do not check if it has run out of oopcodes. However, IIRC that is checked whenever a new statement is executed. (not optimal solution right now, but it works)
http://devs.openttd.org/~zuu/sq2.patch

This comment was imported from FlySpray: https://bugs.openttd.org/task/5561#comment12241

@DorpsGek
Copy link
Member Author

Zuu wrote:

Updated patch for solution 1:
http://devs.openttd.org/~zuu/sq1-v2.patch

This patch caries out the idea to update all callbacks to issue the stack insert in different ways depending on if the callback is called in gen world or when resuming a suspended script.

I'm currently looking at ways to in InsertResult detect if the script is suspended or not so that the vast majority of the patch changes can be removed.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5561#comment12242

@DorpsGek
Copy link
Member Author

Zuu wrote:

Update 2 on the patch for solution 1:
http://devs.openttd.org/~zuu/sq1-v3.patch

I have found a quite trivial way to not have to pass around the information if the script is suspended or not. This reduces the patch size significantly. Both in number of lines changed and reduced coding time overhead for creating future APIs.

Given the amount of changed lines by sq2.patch (mostly indentation, but still makes reviewing hard) compared to sq1-v3.patch, I think that solution 1 is now the primary solution.

I have tested sq1-v3.patch both in single player and in multiplayer with a script that creates 50 goals in world gen and print their ID and then continues to create up to 256 goals in total. In both single player and multiplayer I get a nice debug out put with increasing numbers.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5561#comment12244

@DorpsGek
Copy link
Member Author

Zuu closed the ticket.

Reason for closing: Fixed

In r25305

This comment was imported from FlySpray: https://bugs.openttd.org/task/5561

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) bug labels Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek