Navigation Menu

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

Fix/Feature: Make break on log work for game scripts #5206

Closed
DorpsGek opened this issue Jun 5, 2012 · 14 comments
Closed

Fix/Feature: Make break on log work for game scripts #5206

DorpsGek opened this issue Jun 5, 2012 · 14 comments
Labels
flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Jun 5, 2012

Zuu opened the ticket and wrote:

From users (AI/GS dev) point of view:
- Unpausing the game does no longer continue a script (only way to continue, is the Continue button)
- break on log works also for scripts (that get suspended until you click Continue)
- if you switch between different AIs/GS in the debug window, the Continue button remembers which script that is paused and not paused.
- if a game is saved, it may still run and possible output log messages even if a script has been paused.

Under the hood:
- ScriptInstance now have a flag is_paused. If it is true, ScriptInstance::GameLoop will not execute any squirrel code.
- When Saving, the AI/GS Save() function will still be executed even if is_paused is true.
- Checking of break-on-log strings is still a bit misplaced in GUI-code
- When a break-on-log match is found, the AI/GS is suspended (= all oopcodes become used) and the is_paused flag is set to true. If the game is not paused, it become paused
- When a user click continue, the corresponding script is unpaused. The game itself is only unpaused if all AIs/GS have been unpaused. (and the user has not unpaused the game by eg clicking on the pause button in the main toolbar)
- When a user switch AI/GS to view log, the disabled state of the Continue button is set as !is_paused. The highlighted row is set to -1.

Not fixed:
- This patch do not fix so that the highlighted row and break string is stored per script. That have been left out to not additionally complicate this patch.
- Testing of break on log strings have not been moved from GUI code down to a lower level of the program as has been proposed before.

Attached:
- patch against r24325

Attachments

Reported version: trunk
Operating system: All

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

DorpsGek commented Jun 5, 2012

Zuu wrote:

Fixed some spacing issues in the previous patch.

(I had been checking for spaces at the end of line but forgot tabs at end of line)

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11275

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 5, 2012

Zuu wrote:

Fix: When opening the AI/GS debug window, restore the state of the continue button the same way as when switching "tab" in the window.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11276

@DorpsGek
Copy link
Member Author

DorpsGek commented Jun 5, 2012

Zuu wrote:

Fix: Don't comment out code, remove it!

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11277

@DorpsGek
Copy link
Member Author

frosch wrote:

Updated the diff to HEAD, and fixed a compiler warning.

I think the GUI needs some stronger indicator when a script is paused. Only un-greying the continue-button is not very visible.
Maybe similar to crashed scripts the tab at the top could be colored, maybe yellow?

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11283

@DorpsGek
Copy link
Member Author

Zuu wrote:

I like your suggestion on adding a stronger visual indicator. Pausing a script is after all a fairly important action. I'll take a look into that next time I have time to do so.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11284

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 3, 2012

Zuu wrote:

Changes:
- Brought up to HEAD (minus one unrelated newer revision)
- Paused AIs are indicated by a yellow AI button.

Not fixed:
- Trunk doesn't indicate if a GS is dead by setting the button colour. Therefore, this patch doesn't apply the yellow colour for game scripts because it requires first fixing the trunk-bug/missing feature.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11302

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 3, 2012

Zuu wrote:

I've created a new patch in #5230 that changes the background colour of the GS button when the GS crashes. Here is a new patch that depend on #5230 to mark both AIs and GS yellow if they are paused.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11303

@DorpsGek
Copy link
Member Author

Zuu wrote:

Update to apply with version 2 of the patch in #5230.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11368

@DorpsGek
Copy link
Member Author

11Runner wrote:

Works for me. Thanks Zuu!

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11370

@DorpsGek
Copy link
Member Author

Zuu wrote:

As #5230 have been updated and then committed, I've updated this patch to apply against trunk (r24489).

Additionally I've reviewed all comments to make full sentences in the comments that are added/changed.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11458

@DorpsGek
Copy link
Member Author

Zuu wrote:

Re-read the whole patch and found some minor tweaks.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11459

@DorpsGek
Copy link
Member Author

Zuu wrote:

Tweaked some comments based on feedback on IRC.
Also added a missing != NULL.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11461

@DorpsGek
Copy link
Member Author

Zuu wrote:

Addressed comments:
anyways, + (paused ? COLOUR_YELLOW : COLOUR_GREY); <-- needs an extra TAB

=> Fixed

Zuu: in src/script/script_instance.cpp, 2nd hunk + if (this->is_paused) return; <--can be combined with the line above
Zuu: in script_instance.cpp second chunk, shouldn't the "if (this->is_paused)" check be after the "crashed during saving" check?

=> Fixed using Yexo's suggestion

+ /* Set the paused flag. */ <-- 3rd hunk, this is useless, as it says exactly the same thing as the code at the line below

=> Comment removed

also you've modified the comments for IsSleeping() but not the code, is that correct?

=> That is correct. Explanation (same as told on IRC): If you enter a new tick when the AI/GS is paused, it will never be un-suspended and thus in that suspended-case the check is enough. If you are in a tick when a AI/GS get paused, then in case of eg. a Valuator, there may be a delay from when is_paused is set to true and the AI/GS beeing suspended. Only when the AI/GS have been suspended it is sleeping even if it had its is_paused-flag toggled before.

The chance in src/misc_cmd.cpp looks fishy at first sight, I'd expect it to be moved but it disappears instead

=> That change have been removed and the code in OnInvalidate have been restored to react on invalidate data -2.

Attachments

This comment was imported from FlySpray: https://bugs.openttd.org/task/5206#comment11529

@DorpsGek
Copy link
Member Author

Zuu closed the ticket.

Reason for closing: Implemented

In r24537

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

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay Script labels Apr 7, 2018
This was referenced Apr 7, 2018
Closed
Closed
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/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@DorpsGek