stackgl / glsl-transpiler

Transpile GLSL to JS
http://stackgl.github.io/glsl-transpiler/
175 stars 21 forks source link

Redundant function input args slicing #47

Open dy opened 5 years ago

dy commented 5 years ago

Transpiling

vec4 lerp(vec4 a, vec4 b, float t) {
    return t * b + (1. - t) * a;
}

gives

        function lerp (a, b, t) {
            a = a.slice();
            b = b.slice();
            return [t * b[0] + (1. - t) * a[0], t * b[1] + (1. - t) * a[1], t * b[2] + (1. - t) * a[2], t * b[3] + (1. - t) * a[3]];
        }

which has redundant slicing: slower performance, longer output. Probably we should guard mutation only in case of out qualifiers.

@Pessimistress what do you think?

Pessimistress commented 5 years ago

Consider this function:

bool isVisible(vec4 position_clipspace) {
  position_clipspace.z /= position_clipspace.w;
  return position_clipspace.z > -1.0;
}

Transpiles to

function isVisible (position_clipspace) {
  position_clipspace[2] /= position_clipspace[3];
  return position_clipspace[2] > -1;
};

The input variable should not be mutated according to https://www.khronos.org/opengl/wiki/Core_Language_(GLSL)#Parameters

I acknowledge that this is likely very rare and not good coding practice anyways. I agree the slicing feels like too much overhead than it's worth. Is there anyway to detect mutation and only slice the input in that case?

dy commented 5 years ago

We would have to introduce "horizontal knowledge" for nodes, like constant propagation etc.

Right now we have only "vertical" knowledge: children, parent nodes.

Alternatively, we could try to detect if a function modifies its arguments, that should not be very difficult - just iterate over all nested nodes, check if an argument is engaged in assignment nodes.

Mb we could also figure that out somehow from the scope stats - track variables state.

dy commented 5 years ago

Seems that the project may need refactoring soon, if we find a sponsor. Tech debt accumulates, the code oftentimes vague and unclear, like variables tracking, descriptor init cases etc, needs more purity.

Pessimistress commented 5 years ago

I'm ok with removing the argument slicing for now and maybe adding a note in the readme - we do not have any real world use case that is affected by this.

dy commented 5 years ago

@Pessimistress that's fine, let's keep it. We have many other redundancies.