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

Each execution state needs an I variable #94

Closed
burnsauce opened this issue Sep 5, 2017 · 12 comments
Closed

Each execution state needs an I variable #94

burnsauce opened this issue Sep 5, 2017 · 12 comments

Comments

@burnsauce
Copy link
Contributor

@burnsauce burnsauce commented Sep 5, 2017

1:
L 1 4: SCRIPT 2; A I
2:
L 5 8: B I

A -> 8
B -> 8

I solved this problem with the while loop by tracking per execution level in es_state, which made sense but was not ideal. I is in ss.variables. Does ss.variables.i[EXEC_DEPTH] offend anyone? If not, I can implement a fix that way. I'm amenable to some refactoring if someone sees an abstraction that works better.

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Sep 5, 2017

Counterpoint (and untested on hardware):

1:
L 1 4: SCRIPT 2; A I

2:
I 100

A -> 100

I would expect A to be 100 after executing script 1. With the change you are suggesting it would be just 4 wouldn't it?

@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 5, 2017

@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 5, 2017

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Sep 5, 2017

I still disagree with regards to I, plus it will be a breaking change.

Also, can you explain the issue with IF and SCRIPT for me.

@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 5, 2017

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Sep 5, 2017

Gotcha.

I'm wondering if we should instead think about pushing and popping the execution state then? And track the execution depth via the height of the stack.

Or maybe keeping exec_state_t for the entire execution context, but having a member for things that need pushing and popping via SCRIPT:

e.g.

typedef struct {
    bool if_else_condition;
} nested_exec_depth_t;  // naming things is hard.

typedef struct {
    nested_exec_depth_t nested[EXEC_DEPTH];
    uint8_t exec_depth;
} exec_state_t;

Plus high level functions in state.c to push, pop, and obtain the depth.

@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 5, 2017

@samdoshi
Copy link
Collaborator

@samdoshi samdoshi commented Sep 5, 2017

I think a stack is appropriate. I'll bang it out tomorrow afternoon.

Can you try and add some tests for it (if possible?). Feels like the kind of thing that someone could end up breaking in the future by accident without some safeguards.

@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 5, 2017

burnsauce added a commit to burnsauce/teletype that referenced this issue Sep 6, 2017
If conditions and for loop iterators do not transcend their execution
depth.

Live mode now behaves like this:

> L 1 4: A I
> A => 4
> I => 0

I is 0 because it exists in a different context than the first command.
@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 6, 2017

Working on a test rig now, but SCRIPT now behaves like this:

1:
SCRIPT 1
A + A 1

> A 0
> <F1>
> A => 9
> A 0
> SCRIPT 1
> A => 8

Notes:

  • Upon stack overflow, no new SCRIPT commands may execute, and is considered a user error.
  • Only SCRIPT is blocked upon stack overflow. The remainder of the script runs.
  • Live mode command prompt consumes 1 execution state slot

Breaking behaviour on L: I is transient and associated with a script's execution. This means that it no longer serves as a general-purpose scene variable.

1:
A 0; B 0
L 1 4: SCRIPT 2; A + A I

2:
L 1 4: B + B I

> <F1>
> A => 10 (previously: 16)
> B => 40
@burnsauce
Copy link
Contributor Author

@burnsauce burnsauce commented Sep 6, 2017

I realize that I could design I to have multiple behaviours and be non-breaking.

Essentially, leave I in ss.variables, use internal, exec depth tracked I's in loop mode, then update ss.variables.i to be correct per the old behaviour when the loop is done.

The I operator would check if there is a loop running to return one or the other.

Would that be preferable?

@tehn
Copy link
Member

@tehn tehn commented Sep 12, 2017

for me this doesn't represent a huge breaking issue. i doubt that there are a lot of people that have a lot of scripts relying on calling SCRIPT within L using the I variable.

it makes sense to me to make the I var local to the loop. if someone wants to get I into a script, just mind the ordering:

1:
L 1 4: A I; SCRIPT 2

2:
B ADD B A

what's most important to me is predictable, intuitive behavior. if we have to explain a detail in the docs, that's fine.

@tehn tehn closed this in ad358cd Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants