Backwards Compatibility for 5.0.0-rc2 and Container.render method

[aaclayton]

Hi PIXI team,

Firstly thank you so much for your work on this software and for all the effort that has gone into 5.0.0.

I got my first chance to try out the release candidate build this evening and struggled for quite some time with some silent breakages in my application. The root cause ended up being the following:

I had extended the base PIXI.Container class and implemented a render() method on my subclass (perhaps this was not advisable in the first place... but it made sense to me at the time) which performed some application specific steps related to drawing that particular container. Now, with 5.0.0, the base Container class defines its own render(renderer) method with behavior that is critical to core functionality.

https://github.com/pixijs/pixi.js/blob/9076a8983ef73ef00152b68bb98cc0ea4083d27a/packages/display/src/Container.js#L488

My user experience was that nothing was working. My containers were not being drawn and it was quite tricky to debug due to the lack of any error message or warning.

Some thoughts:

1. I think it is completely reasonable for core PIXI classes to reserve the render() method for internal use and I am happy to change my own implementation, but it would be ideal to spare other users from several hours of confusion while they attempt to identify the root cause.
2. Given that PIXI has been around for some time and that users may have many existing PIXI.Container subclasses which they would hope to migrate to 5.0.0, would it be possible to include some sort of sanity check on the Prototype that raises a warning of this critical render() method has been changed or overridden?

Steps to Reproduce

1. Define a subclass of PIXI.Container (presumably under v4.x) and implement a render() method for that subclass which does.... something.

2. Upgrade to PIXI 5.0.0.

3. Observe that your custom Container no longer functions as expected with no warnings or errors thrown.

Environment

• pixi.js version: 5.0.0-rc2
• Browser & Version: Any
• OS & Version: Windows x64

[ivanpopelyshev]

Oopsie. Same as "app.stop" but this thing is bigger.

I was neutral about that change.
Well, actually that's a good idea. Lets go back to renderWebGL, please? @GoodBoyDigital

Or maybe renderElement and renderContainer will be better, I agree that_render and render is a bad idea.

[bigtimebuddy]

In v4, we had functions named _renderWebGL and _renderCanvas. We dropped the "WebGL" to make rendering WebGL the first-class default and kept things like _renderCanvas. It makes sense that there'd be issues if you were overriding here and not calling the correctly renamed private _render method.

In general, private methods don't have any guarantees that we won't change. However, we could add a deprecated warning for _renderWebGL method to make this clearer.

[bigtimebuddy]

@aaclayton did the above renaming (_renderWebGL => _render) fix your issue with your render override not working?

[aaclayton]

Hi @bigtimebuddy - I don't believe so.

The issue (per my understanding, I may be wrong) is that in PIXI v4 the Container object did not have a render method at all -- so this public method was "available" for a subclass to implement to perform some behavior.

Now in v5, the Container has a render method in it's public API which is expected to perform some specific role of rendering the container using WebGL.

If, as in my case, a user of PIXI has defined the render method themselves assuming it was safe to do so, their code will not function under v5. It has nothing to do with the underlying private _render vs _renderWebGL method naming, it's all about the public render method defined on line 488.

https://github.com/pixijs/pixi.js/blob/dev/packages/display/src/Container.js#L488

Does that clarify?

EDIT: To be even a bit more specific -- the problem was that in v4 I was not making a conscious choice that I was overriding a method, I was just defining a method that was not already defined. The ground has shifted beneath me so that now that method IS overriding a base/core functionality. So the assumptions I was making when defining my render method now have much more significant consequences that I realized when making that choice.

[bigtimebuddy]

Ahh okay, I understand now. Thanks for clarifying.

Going from v4 to v5, we renamed Container's renderWebGL to render. We provide a deprecation for this so that users who called this directly will now get a helpful warning.

There is, however, no way for us to prevent your issue. Any user could've created any custom method or property that is now used v5. We really can't doing anything defensively to check internal name collisions or prevent a method from being overridden. I'm sorry that your thing wasn't working as expected; the change is to rename your function to something else unfortunately.

The best I can offer you is a note in some migration documentation (which is currently work in-progress).

[aaclayton]

Makes sense, and as I said in my original post, I am fine with changing my code and moving to a different method name.

My concern is just around empathy for other developers as this issue was very difficult for me to debug - my containers were not rendering, there was no error message, there was no warning, just an unexpected failure.

I think if you make this change it will be very important to help users who are migrating to v5 to understand that this is a key failure mode that could occur if they have defined this method to spare other users a frustrating experience.

[bigtimebuddy]

Totally agree with you. We're doing our best to push things forward and not break folks as much as possible.

[bigtimebuddy]

Added a note here: https://github.com/pixijs/pixi.js/wiki/v5-Migration-Guide which includes a link back to this issue. Feel free to modify the wiki or add any additional notes.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.