cts icon indicating copy to clipboard operation
cts copied to clipboard

Refactor Mixins and other inheritance

Open greggman opened this issue 1 year ago • 2 comments

I'm running into issues that it's often hard to refactor or change things because of the hierarchy of inheritance with

GPUTest
ValidationTest
ShaderValdiationTest
TextureTestMixin

It's not clear why many of these things are implemented via inheritance instead of as just standalone functions or static functions on a class or a collection of object methods

For example

TextureTestMixin ...

    expectTexelViewComparisonIsOkInTexture(
      src: GPUTexelCopyTextureInfo,
      exp: TexelView,
      size: GPUExtent3D,
      comparisonOptions = {
        maxIntDiff: 0,
        maxDiffULPsForNormFormat: 1,
        maxDiffULPsForFloatFormat: 1,
      }
    ): void {
      this.eventualExpectOK(
        textureContentIsOKByT2B(this, src, size, { expTexelView: exp }, comparisonOptions)
      );
    } 

Can just as easily be implemented as

    function expectTexelViewComparisonIsOkInTexture(
      t: GPUTest,
      src: GPUTexelCopyTextureInfo,
      exp: TexelView,
      size: GPUExtent3D,
      comparisonOptions = {
        maxIntDiff: 0,
        maxDiffULPsForNormFormat: 1,
        maxDiffULPsForFloatFormat: 1,
      }
    ): void {
      t.eventualExpectOK(
        textureContentIsOKByT2B(t, src, size, { expTexelView: exp }, comparisonOptions)
      );
    } 

But now it's not inherited.

If it's important we could put bunch of these on objects/classes so there is less importing required.

// texture_test_helpers.ts
export default {
  createTextureFromTexelView(...),
  expectTexelViewComparisonIsOkInTexture(...),
  createTextureFromTexelViewsMultipleMipmaps(...),
  ...
}

Then

import tHelpers from './texture_test_helpers.ts`

...
  const tex = tHelpers.createTextureFromTexelView(...);

In any case, it feels like separating functions out from the inheritance hierarchy makes them more usable in more places.

greggman avatar Feb 27 '25 18:02 greggman

It's not clear why many of these things are implemented via inheritance instead of as just standalone functions or static functions on a class or a collection of object methods

IIRC it was a (possibly misguided) attempt to make things more discoverable. I found contributors were almost never finding standalone helper functions that could be relevant, whereas they were finding ones that were in GPUTest and ValidationTest. I also thought I would end up refactoring things a bit more so that some mixins depended on others, but I don't think that really happened.

A class of static functions or something like that might solve the problem just as well. The only real difference is the addition of a t: GPUTest parameter, or whatever.

kainino0x avatar Feb 28 '25 05:02 kainino0x

Another thing we could do is promote a pattern of

import fi as * from '../../../format_info.ts';

...

   fi.isTextureFormatColorRenderable(...)

Maybe not a good example because format_info.is isn't one of the these classes with a bunch of utility functions on it but, it illustrates a pattern we might find more useful. It would also mean less churn in the import area.

So maybe not important to organize them as an object of functions or a class of static functions.

greggman avatar Mar 08 '25 02:03 greggman