TSL transpiler turns multiplication with signed float into subtractions

[porst17]

Description

The GLSL to TSL transpiler turns multiplications with negative numbers into subtractions if they are not surrounded by parenthesis.

The GLSL code

float correct = 1.0 * (-1.0);
float wrong = 1.0 * -1.0;

is transpiled to the obviously wrong result:

// Three.js Transpiler r177

import { float, mul } from 'three/tsl';

const correct = float( mul( 1.0, float( - 1.0 ) ) ).toVar();
const wrong = float( 1.0 - 1.0 ).toVar();

The same issue occurs for vector (and matrix?) data types when they are multiplied by a negative number.

Reproduction steps

1. Copy the above example into the TSL transpiler.

Code

GLSL code that gives wrong transpilation result:

float correct = 1.0 * (-1.0);
float wrong = 1.0 * -1.0;

Live example

TSL transpiler (copy the code from above).

Screenshots

No response

Version

r177

Device

Desktop

Browser

Firefox

OS

MacOS

[cmhhelgeson]

Tests. I've listed the examples in descending order of each operator's 'precedence'. Note how when the operator's precedence is less than or equal to '-', the operations begin to break down.

// Slice of Precedence Array                                                                  //works......................//doesn't work.....
const precedenceOperators = ['*', '/', '%', '-', '+', '<<', '>>', '<', '>', ...].reverse() =  ['>', '<', '>>', '<<', '+', '-', '%', '/', '*'];

// GLSL
wrong = 1.0 > -1.0;
wrong = 1.0 < -1.0;
wrong = 1.0 >> -1.0;
wrong = 1.0 << -1.0;
wrong = 1.0 + -1.0;
// Starts to break down here
wrong = 1.0 - -1.0;
wrong = 1.0 % -1.0;
wrong = 1.0 / -1.0;
wrong = 1.0 * -1.0;

// TSL
wrong.assign( greaterThan( 1.0, float( - 1.0 ) ) );
wrong.assign( lessThan( 1.0, float( - 1.0 ) ) );
wrong.assign( shiftRight( 1.0, float( - 1.0 ) ) );
wrong.assign( shiftLeft( 1.0, float( - 1.0 ) ) );
wrong.assign( add( 1.0, float( - 1.0 ) ) );
// Starts to break down here
wrong.assign( float( - 1.0 ).sub( 1.0 ) );
wrong.assign( 1.0 - 1.0 );
wrong.assign( 1.0 - 1.0 );
wrong.assign( 1.0 - 1.0 );

[sunag]

Maybe the trick here is to detect when the expression has two arithmetic operators in sequence, this shouldn't be possible anyway.

[Makio64]

I had similar issue with the minus priority doing the tsl-operator, it's such a tricky one.

Maybe adding this kind of unit test would really benefit to tsl-node & tsl-transpiler :
https://github.com/Makio64/vite-plugin-tsl-operator/blob/main/test/index.test.js

[cmhhelgeson]

Maybe the trick here is to detect when the expression has two arithmetic operators in sequence, this shouldn't be possible anyway.

I think a good start could be having a prev token constant that gets updated on each parseToken call. In the situation that is emerging, our current 'precedence' operator is '-', but we are reading a ''. The commented check will thus skip the '', because '-' != '*'

const parseToken = ( i, inverse = false ) => {
     const token = tokens[ i ];
     groupIndex += getGroupDelta( token.str );
     if ( ! token.isOperator || i === 0 || i === tokens.length - 1 ) return;
     // '*' is not a case ender, but it is not equal to the current operator of, so it gets skipped in the parseToken loop,
     // leading the decoder to believe it has parsed 1.0 - 1.0 instead of 1.0 * (-1.0)
     if ( groupIndex === 0 && token.str === operator ) {

By maintaining the prevToken, we can try to implement some logic that catches instances like this (managing proper prevToken state is probably more complicated than as presented below, but I think general idea is sound)

// Assume current operator is '-'
for (const operator of precedenceOperators) {
     // Currently unassigned, but will always get reassigned on first iteration of parseToken loop
     // since i == tokens.length - 1 will always return
     let prevToken= null;
     
     const parseToken = ( i, inverse = false ) => {
          const token = tokens[ i ];
          groupIndex += getGroupDelta( token.str );

           if ( ! token.isOperator || i === 0 || i === tokens.length - 1 ) {
               prevToken= token;
               return;
           }
     
           // token.str of '*' is not equal to current operator of '-'
           if ( groupIndex === 0 && token.str === operator ) {
           // Standard logic

           // But we can now execute separate logic if the lastToken was an operator 
           } else if (prevToken.isOperator)  {
           // New logic for consecutive individual operators like (1.0 * -1.0) or (1.0 / -1.0)
           // Parse expression with * or / as the delimiter between the left and right side expressions

           }

[sunag]

@Makio64 Yes, that would certainly be great and it will be necessary at some point.

@cmhhelgeson I added a simple skip for now, I'll start developing a flow control for the transpiler, so we can do the syntax optimizations.