Cannot override methods when extending WebGLRenderer

[marcofugaro]

Description

In my case, I have a CustomRenderer class that extends WebGLRenderer. I want to dispose of other things in the dispose() call, so I created an override method that disposes of my resources and also calls super.dispose().

However, in WebGLRenderer, the method doesn't ever get called, and instead the WebGLRenderer.dispose() is called directly.

class CustomRenderer extends WebGLRenderer {
  dispose() {
    super.dispose()
    // the code here is never run..
  }
}

const renderer = new CustomRenderer()
renderer.dispose()

This is because the WebGLRenderer.dispose(), is not a class method per se.

Reproduction steps

1. Create a class that extends WebGLRenderer
2. Create a method that overrides a WebGLRenderer method
3. Instance the class and call the method

Code

import { WebGLRenderer } from 'three';

class CustomRenderer extends WebGLRenderer {
  dispose() {
    super.dispose()
    
    // custom code goes here...
    console.log('1. I should be logged to the console')
  }
}

const renderer = new CustomRenderer()
renderer.dispose()

console.log('2. The log n. 1 should be present above this ^')

Live example

• jsfiddle

Screenshots

No response

Version

r150

Device

No response

Browser

No response

OS

No response

[marcofugaro]

Looking at the WebGLRenderer class more deeply, I believe this can be achieved when we can use private class properties.

[agargaro]

You can override in this way but of course is not clean

class CustomRenderer extends WebGLRenderer {

  constructor() {
     super();
     const disposeBase = this.dispose;
     this.dispose = () => {
         disposeBase();
         // custom code goes here...
         console.log('1. I should be logged to the console')
     };
  }
}

[marcofugaro]

Yup, I ended up calling the dispose() method a different way.

[epreston]

Grab a reference to the original dispose method, replace dispose method on original, use the reference in the body of your replacement to do an effective call to super.

[mrdoob]

@marcofugaro Did #25599 address this?

[marcofugaro]

No, not yet. After #25599 we are still defining dispose as a class property.

three.js/src/renderers/WebGLRenderer.js

Line 574 in 8571a82

this.dispose = function () {

To fix this issue we need to define dispose as a class method.
But we can't do that yet because we would need to use private class properties to avoid exposing internal WebGLRenderer stuff to the user.

[Mugen87]

We can't use private class fields since we want to stick to ECMA2018. So if WebGLRenderer should really be refactored like suggested, private properties have to use the _ prefix to mark them as private (see #25665).

[marcofugaro]

Yup, should we refactor using _ now or should we wait until we can use a new ECMA version in your opinion?

[Mugen87]

Yup, should we refactor using _ now or should we wait until we can use a new ECMA version in your opinion?

We have lately decided to stick to ECMAScript 2018 (see #25185) and I would prefer to not revisit this topic in the next time.

[marcofugaro]

@Mugen87 you misread my question. I asked if we should use _ like you did in your PR (we can do this with ECMA2018), or wait until we can use a new ECMA version and use private class fields.

[Mugen87]

Sorry, you're right! We can use the _ approach now and later replace it with private class properties. So we don't have to wait.

I'm just hesitating since refactoring big classes always introduces the risk of breaking something. But I guess sooner or later the mentioned inheritance issue with the renderer would come up again. My hope was to avoid the rewrite as long as possible^^.