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

Scripts run 3 times on windowed mode #247

Open
rlafuente opened this issue Jun 3, 2019 · 14 comments
Open

Scripts run 3 times on windowed mode #247

rlafuente opened this issue Jun 3, 2019 · 14 comments

Comments

@rlafuente
Copy link
Contributor

This is evident with print output -- code is executed 3 times and variables don't reset between runs. Serious stuff :-/

@rlafuente
Copy link
Contributor Author

With this script

print(colormode())
colormode(HSB)
print(colormode())

I get this output:

rgb
hsb
hsb
hsb
hsb
hsb

The variables aren't reset between runs, which is not good.

@rlafuente rlafuente added the bug label Jun 3, 2019
@rlafuente
Copy link
Contributor Author

However, with this

print(colormode())

def setup():
    colormode(HSB)
    print(colormode())

gives

rgb
hsb
hsb
hsb

I have absolutely no clue what's going on here!

@stuaxo
Copy link
Contributor

stuaxo commented Jun 3, 2019

Obviously having output 3x is wrong.

As for colormode being persistent, I would run this in nodebox1 before being sure it's a bug.

@stuaxo
Copy link
Contributor

stuaxo commented Jun 3, 2019

The bit of code that does the running is a bit complex, (and needs some tests).

Our suspects all live in grammar.py in the Grammar class:

def _should_run(self, iteration, max_iterations):
    ''' Return False if bot should quit '''
    ...

One thing this looks at is self._dynamic - which is set if the bot has draw function.

def _run_frame(self, executor, limit=False, iteration=0):
""" Run single frame of the bot```
    :param executor:  The LiveExecutor which can rollback on errors
 def run(self, inputcode, iterations=None, run_forever=False, frame_limiter=False, verbose=False)
    ...

It looks like there might be a bit of duplication between the logic in run and _run_frame.

It could be worth putting some print statements in these places, especially checking _should_run, _dynamic and what iterations is.

@stuaxo
Copy link
Contributor

stuaxo commented Nov 26, 2019

The first case running 3x is strange, but we can probably work it out.
The second case runs the code one extra time.

If this can be reproduced without the GUI it should be easy(ish) to make a test, if it needs the GUI it would be more tricky.

@stuaxo
Copy link
Contributor

stuaxo commented Nov 26, 2019

Further notes:

Bug only occurs when running in windowed mode, so this may be GUI events causing extra drawing.

sbot 3.bot
$ sbot -w 3.bot
rgb
hsb
hsb
hsb
hsb
hsb
$ sbot 3.bot -o blah.png
rgb
hsb

@stuaxo stuaxo pinned this issue Feb 10, 2020
@stuaxo
Copy link
Contributor

stuaxo commented Feb 10, 2020

Pinned - we need a way to test running under Gtk headlessly + get some tests around execution to solve this.

@stuaxo stuaxo added the test label Feb 10, 2020
@stuaxo
Copy link
Contributor

stuaxo commented Feb 25, 2020

Tests now run under Gtk, so implementing tests for this should be fairly straightforward.

@stuaxo
Copy link
Contributor

stuaxo commented May 3, 2020

Trying to write some tests for this + realising current code is a bit insane, so some notes:

First question is how to decide if we are an animation.

https://plotdevice.io/tut/Animation
to recognize your script as an animation, simply define a function called draw() somewhere in your code.

Q. to verify
http://support.nodebox.net/discussions/nodebox-1/91-how-does-shoebot-1-decide-to-render-a-single-image-or-run-an-animation

@stuaxo
Copy link
Contributor

stuaxo commented May 3, 2020

Probably need some idea of animation state that lives outside the bot namespace to do this in a sane way.

Then the two kinds of bot:

  • No draw function (not animated unless it calls speed > 0)

  • Has a draw function (animated at default speed unless it calls speed = 0)

Can be handled sanely.

Obviously needs tests :)

@stuaxo
Copy link
Contributor

stuaxo commented May 8, 2020

Nodeboxes design of having a "runner" is a nice way of encapsulating this logic, wish I'd looked there ages ago.

@stuaxo stuaxo mentioned this issue May 8, 2020
11 tasks
@stuaxo
Copy link
Contributor

stuaxo commented Oct 7, 2020

I'm going to be annoying and add this to the release blockers for 1.4.0, reason being that I have the testing framework setup for this partially done on my computer.

Some lifetime fixes are outstanding when running in the GUI:

  • It doesn't close one Window before opening the next one
  • It needs a way to force the bot to close if it decides it wants to run forever).

@rlafuente rlafuente changed the title Scripts run 3 times Scripts run 3 times on windowed mode Apr 7, 2021
@stuaxo
Copy link
Contributor

stuaxo commented May 27, 2021

Goodness me! This bug has triggered a lot of changes - some pretty large - and we are still not all the way there.

Here is a little writeup:

This bug occurs in the main loop, which sits at the heart of Shoebot.

When this bug was reported our test framework was pretty non-existent, so changing the main loop would have been very risky.

We added a new test framework, that was able to run bots in headless and windowed mode, we included useful features like being able to verify visual output.

This helped us find many other bugs, implement many other features (with tests) and make our CI much more useful.

Once the test suite matured replacing the main loop was relatively painless.

This made the code easier to read and Shoebot was able to run code in the global scope once as opposed to 3x.

As is often the case, this revealed a new bug or two:

In Shoebot you can change the size of the window with the size(width, height) command.

Shoebot renders scripts at the window size. Unfortunately, if the user calls size(...), things aren't in sync until the next frame and Shoebot renders at the size of the Window for the previous frame.
This bug manifests as the user seeing their output in the top-left corner of the Window.

Currently, Shoebot's graphics backend can only output each frame once.
As a workaround shoebot renders the first frame one more time.
Shoebot 1.4 now executes the first frame twice instead of 3x an improvement of one frame though not the full fix.

Future work:
The test framework makes it safe to make changes to the renderer and fix this bug properly.

Rewriting our main loop to better separate concerns (running scripts, handling events) made it easier to understand this bug.
This work highlighted the possibility of removing a lot of glue-code in Shoebot by making better use of events.

Once that work is complete Shoebot, will be free of a persistent bug and, with any luck: a little more maintainable.

@stuaxo stuaxo mentioned this issue Feb 8, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants