Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Mixins and other inheritance #4226

Open
greggman opened this issue Feb 27, 2025 · 2 comments
Open

Refactor Mixins and other inheritance #4226

greggman opened this issue Feb 27, 2025 · 2 comments

Comments

@greggman
Copy link
Contributor

greggman commented Feb 27, 2025

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.

@kainino0x
Copy link
Collaborator

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.

@greggman
Copy link
Contributor Author

greggman commented Mar 8, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants