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

Handle side-effects in expression evaluation #241

Merged
merged 2 commits into from
Oct 30, 2023
Merged

Handle side-effects in expression evaluation #241

merged 2 commits into from
Oct 30, 2023

Conversation

djeedai
Copy link
Owner

@djeedai djeedai commented Oct 30, 2023

Ensure that expressions with a side-effect, like rand(), are evaluated only once even if they're referenced multiple times in a module. Otherwise each evaluation duplicates the side effect, which in the example of rand() yields a different value even though the expression is the same.

Add Expr::has_side_effect() to determine if an expression has any side effect. Currently only expressions returning a random value have a side effect.

To ensure a unique evaluation per expression, add an evaluation cache to the implementations of EvalContext, saving the evaluated string associated with an ExprHandle.

Add to EvalContext some helpers:

  • make_local_var() to create a unique variable name, for storing intermediate evaluations.
  • push_stmt() to write the statement which actually stores the evaluation of an expression into a local variable before it's referenced.

Remove the module() and expr() from EvalContext, and move the mutable Module reference out of that context, passing it explicitly instead. This allows acquiring a mutable reference to both the context and the module at the same time, which is not possible if the module reference is stored as a context field.

Add EvalContext::make_fn() to generate a function with a temporary function-local context. This ensures that functions which use expressions with side effects have their local statements emitted inside the body of the function, and not at the call site where they're not available.

Fix several bugs in some older modifiers which were using hand-crafted code not properly parenthesized, which could lead to invalid expression evaluation due to operator priorities in WGSL:

  • SetPositionCircleModifier
  • SetPositionSphereModifier
  • SetVelocityCircleModifier
  • SetVelocitySphereModifier
  • SetVelocityTangentModifier

Bug: #240

Ensure that expressions with a side-effect, like `rand()`, are evaluated
only once even if they're referenced multiple times in a module.
Otherwise each evaluation duplicates the side effect, which in the
example of `rand()` yields a different value even though the expression
is the same.

Add `Expr::has_side_effect()` to determine if an expression has any
side effect. Currently only expressions returning a random value have a
side effect.

To ensure a unique evaluation per expression, add an evaluation cache to
the implementations of `EvalContext`, saving the evaluated string
associated with an `ExprHandle`.

Add to `EvalContext` some helpers:
- `make_local_var()` to create a unique variable name, for storing
  intermediate evaluations.
- `push_stmt()` to write the statement which actually stores the
  evaluation of an expression into a local variable before it's
  referenced.

Remove the `module()` and `expr()` from `EvalContext`, and move the
mutable `Module` reference out of that context, passing it explicitly
instead. This allows acquiring a mutable reference to both the context
and the module at the same time, which is not possible if the module
reference is stored as a context field.

Add `EvalContext::make_fn()` to generate a function with a temporary
function-local context. This ensures that functions which use
expressions with side effects have their local statements emitted inside
the body of the function, and not at the call site where they're not
available.

Fix several bugs in some older modifiers which were using hand-crafted
code not properly parenthesized, which could lead to invalid expression
evaluation due to operator priorities in WGSL:
- `SetPositionCircleModifier`
- `SetPositionSphereModifier`
- `SetVelocityCircleModifier`
- `SetVelocitySphereModifier`
- `SetVelocityTangentModifier`

Bug: #240
@djeedai djeedai added C - bug Something isn't working C - breaking change A breaking API or behavior change A - expressions Change related to the Expression API labels Oct 30, 2023
@djeedai djeedai merged commit a58475e into main Oct 30, 2023
12 checks passed
@djeedai djeedai deleted the u/side-effect branch October 30, 2023 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - expressions Change related to the Expression API C - breaking change A breaking API or behavior change C - bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant