djeedai / bevy_hanabi / 17957503606

66%

Build Type

push

github

Committed by web-flow

Commit Message

Check the results of `has_side_effect` for binary expressions. (#497)

Currently, if the output of `UniformRand` is used in multiple dependent
expressions, a new random number will be generated every time. This is
at odds with the behavior of `BuiltInOperator::Rand`. The code *tries*
to account for this by implementing `has_side_effect()` on binary
expressions, but this is never actually checked. That's because
`has_side_effect()` is only examined for *built-in* expressions, which
`UniformRand` is not one of.

This commit factors the logic to create a local variable if there are no
side effects out of `BuiltInExpr` into a separate function and calls
that function in `Expr::Binary` and `BuiltInExpr`, fixing the issue.

A separate issue is that `Expr::has_side_effect()` is perhaps too
aggressive, in that binary operators check to see whether their LHS and
RHS are side-effecting and mark themselves as side-effecting if they
are, when in fact only the operator itself determines whether the
expression is side-effecting for the purposes of generating new
temporaries. This causes many expressions to be marked as side-effecting
that actually aren't. I opted not to fix this issue in this PR to keep
this PR small, as it only results in more temporaries being generated,
which shader compilers will clean up.

Run Details

19 of 22 new or added lines in 1 file covered. (86.36%)

1 existing line in 1 file now uncovered.

4917 of 7399 relevant lines covered (66.45%)

449.07 hits per line