I notice that there is an update method for the uniforms. This would create problems for the framework which tends to assume that Shader/Filter objects are immutable. These objects get stored in the framework and reused on subsequent frames. If their values can change without any reconstruction of the associated widgets then that could cause a problem.

I would consider using a constructor to construct a new shader from an existing shader which reuses its program, but contains new uniforms.

Tagging @Hixie on the issue of FragmentShader objects implementing the mutable method "update(new uniform values)" and how that might break assumptions in the framework.

@flar is correct that Shaders should be immutable.

Separate from this, there's the extra issue that you can't implement hashCode and operator== on a mutable class in any case. That's just semantically incorrect. Equality of objects must be consistent in time.

My suggestion for hashcode/== would be to back off to identity implementations for now and to change the update() method to any of the following:

• FragmentShader withUniforms(...uniforms...) method
• FragmentShader.withUniforms(FragmentShader original, ...uniforms...) factory (alongside existing constructor, or a constructor that only provides the program and assumes default uniforms)
• Break the Spirv encoder out into a new object and make the constructor FragmentShader(FragmentProgram, ...uniforms...)
• Only provide the current constructor, but then you can't amortize the translation of the Spirv program.

Note that if you break the program out into a separate object then hashcode/== could be implemented with identity on the program and array comparison on the uniforms. This would assume that developers would cache the program for reuse since it has a (potentially) expensive translation and so identity hash and == would work well for it, but if they frequently change the uniforms and the uniform arrays are small, then keeping them stored in the Dart object would allow us to hash and compare them directly.

(Sorry for so many edits to my last comment. I'm done with the rewrites now.)

Another way to view this is that things like ImageFilter.blur are specifying a specific predetermined program and giving it new values that tend to be implemented as uniforms. ImageShader is a specific program that only needs an image to bind as the source pixels and a few parameters that get turned into uniforms for that program.

As such, FragmentShader(Program, ...uniforms...) becomes an instantiation of a specific program specified elsewhere (the Program constructor) with a given set of uniforms. This then mirrors the built-in GPU-shader-based classes better.

Also, one could potentially replace the FragmentShader constructor with FragmentProgram.createShader(...uniforms...)

At this point I'm leaning strongly towards having FragmentProgram be its own class and towards having it be a factory class for the Shader objects themselves instead of a FragmentShader class with a constructor.

This would help steer the developer towards considering the Program as something they create and reuse (since it is a factory class), and we could implement == on the constructed shaders easily by remembering the array of uniforms.

Shader FragmentProgram.createShader(...uniforms...) returns an internal subclass of Shader with a handle to the program and the remembered uniforms and implements hashcode/== fully.

Thoughts on this style @Hixie ?

Seems reasonable.

Thank you both for looking into this, I appreciate the examples!

It sounds like using a factory pattern is the way to go given that Shader instances must be immutable, I wasn't previously aware of that - makes sense. I can draft a new PR for that, I agree with the last style given by @flar of a FragmentProgram or similar with createShader and a private Shader implementation.

In the meantime, I think it would make sense to close this PR.

Separate from this, there's the extra issue that you can't implement hashCode and operator== on a mutable class in any case. That's just semantically incorrect. Equality of objects must be consistent in time.

That makes sense - is this written down somewhere? I was following the Dart documentation for hashCode and operator== which mentions that equality can change only "if one of the objects was modified".

https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#common-boilerplates-for-operator--and-hashcode mentions "If you do override equality, you should use @immutable on the class hierarchy in question" but in general the reason you don't want operator== or hashCode to ever change during the lifetime of an object is that they mostly exist to allow the objects to be put in Maps or Sets, and if they change identity while in a Map or Set things will just break horribly (e.g. you won't be able to find the instance anymore since its hash code will be different).