Add the property isEnabled to the class MaterialPluginBase?

Feature requests
1

Heya, I wonder if maybe we should add code like this for the isEnabled property, taken straight from the Material Plugins doc/examples, to the base class? Because it seems like generic code that ends up getting copied to plugins the same way each time. CC @Evgeni_Popov, @sebavan :slight_smile:

  get isEnabled() {
    return this._isEnabled;
  }

  set isEnabled(enabled) {
    if (this._isEnabled === enabled) {
      return;
    }
    this._isEnabled = enabled;

    this.markAllDefinesAsDirty();
    this._enable(this._isEnabled);
  }

  _isEnabled = false;
2

I am not sure as it is not mandatory to have it:

3

but we could at least factorize the content of the setter in a base protected function so it is easier to add.

4

Hmm IDK what advantage that would have over just inheriting the property and not using it thou. It sounds unnecessarily complicated to me but I could be missing something of course… Either way it would be nice to avoid manually duplicating that code IMO as it seems fairly common to want it even thou plugins can certainly be made without it. :slight_smile:

5

Cause it would feel like a bug if it is inherited but unused. You could set isEnabled to false which would basically do nothing which is what I would like to avoid by providing a protected way. It would only be public if willingly exposed meaning you do the necessary work to support it.

1 Like
6

Aaah I was thinking to explain that in the property’s API doc, that you need to still implement the rest of it, but def agree your way of opting into it sounds better now that you explained the issue more. :smile:

7

OTOH thou maybe it would be better to do it in a subclass that implements the behavior generically (it could prob even handle ALL of it, including auto-wrapping all the shader code in #ifdef statements) and then just inherit from that class if the plugin needs that behavior. Maybe I’ll try this external approach on second thought and just see how it goes. :slight_smile:

1 Like