engine
engine copied to clipboard
playcanvas
[BREAKING] Global app removal
This PR removes the inclusion of the global app reference and uses the static property on app base to store all application instance references including the active application.
API changes
- Removes
pc.app
Reasoning
When creating projects with multiple references, it is unclear which reference is held in pc.app. This should be acquired by using the AppBase.getApplication method which allows specific applications to be acquired based on canvas id or if no id is provided then the current active application.
Oh most definitely it will... counting the number of client and projects of ours we may need to check/update.
So we need to update 16 active projects of ours after this.
- What do we gain after this?
- Is it really necessary?
- Is it possible to make this change but still people allow to use pc.app?
The change makes sense if you want to support multiple playcanvas instances on the same page. I would suggest to keep this backwards compatible by just always making pc.app point to the last created instance. That way it won't break all these projects that only use one instance.
There are so many references to pc.app in the forums, older tutorials and most likely in many user projects 😇. While it's possible to apply a patch just after the engine has loaded for projects that use pc.app, do consider @erikdubbelboer 's suggestion.
This will likely to break a lot of people's projects.
Yes but this will be included in a non forced update along with other breaking changes
So we need to update 16 active projects of ours after this.
- What do we gain after this?
- Is it really necessary?
- Is it possible to make this change but still people allow to use pc.app?
Removes ambiguity when dealing with multiple applications. Having a global pc.app does not indicate which of these applications it refers to. Again this change will be included in a non-forced engine editor update which you will have to upgrade to by choice.
Is non-forced updates a new thing that the editor is going to support? Cause right now we're always forced into all updates.
Currently, it is already possible to create multi-app apps, and devs who do that will have references to apps stored and used as required. So I would say removing pc.app would not affect them at all, but will definitely affect everyone else, like 98% of single-app apps.
Convenience when debugging from console - is a massive.
So if the only reason of removing pc.app is to reduce confusion for multi-apps, then I would suggest to improve docs for multi-app cases, and not remove pc.app.
Is there any other reason for its removal?
I also raise concern on this breaking change. Unless it is needed for another feature, we really shouldn't break this for the reasons above
If this does go ahead, can we at least make it a deprecation?
i see where you're coming from, importing app is very dangerous and error prone. Might I suggest an assertion error/warning + deprecation when accessing pc.app when multiple apps are present. You could always deprecate it then and rename it to make it clear what its for e.g. pc.currentApp and throw an error explaing what you need to do when accessing pc.app for old tutorials etc
I am probably an odd one, but I've never used pc.app. Was it an old way of getting an application instance? Anyhow, I am curious about "other breaking changes" that @kpal81xd mentioned.
@LeXXik
https://github.com/playcanvas/engine/pull/6407 https://github.com/playcanvas/engine/pull/6584
And actually thinking about it, I think what @kpal81xd means by non forced update is that the next minor/major release is the build with the WebGL 1 support removed
https://forum.playcanvas.com/t/phase-out-of-the-webgl1-support/35705
So the current version of the engine (1.71) will always be supported in the engine going forward
Despite this, I still think that pc.app should not be removed unless it's blocking a feature or consider a polyfill for the editor only perhaps?
Currently, it is already possible to create multi-app apps, and devs who do that will have references to apps stored and used as required. So I would say removing pc.app would not affect them at all, but will definitely affect everyone else, like 98% of single-app apps.
Convenience when debugging from console - is a massive.
So if the only reason of removing pc.app is to reduce confusion for multi-apps, then I would suggest to improve docs for multi-app cases, and not remove pc.app.
Is there any other reason for its removal?
pc.app should not be relied on for access to the application instance it should be on the user to correctly manage their application reference as opposed to relying on a global definition of the application. For debugging yes it is useful that is the only reason I would consider leaving it in.
As for affecting other applications this change will be alongside other breaking changes in a non-forced update.
non-forced update
Can you please provide a definition of it? There were no such updates before, so is this different version that users can migrate to, is it a separate version that users use one or the other, will it be forced after some time, what is it?
Many people and projects rely on pc.app, and it is up to docs to provide meaningful info on when it is not recommended.
The reason people use it - is because it is useful and convenient. So there should be a strong reason for its removal, otherwise, it just makes devs lives less convenient and introduces change for no technical reason.
I'm not sure I see what this solves. We used to have a private pc.app that would return a last created app.
Here we have a public AppBase.getApplication(id) which returns the same value when no ID is passed in.
So it only changes the interface, but not solve the actual problem of having a global access to the last created app.
I think the goal here should be to remove any way for the user to obtain global/static reference to the current application, as that in any form causes the problem for multi-app users. Where the app is required by the engine (for example the Entity constructor, but also Asset I think and others, the app instance should be provided by the user, instead of the engine using last created application. But that is a lot larger breaking change unfortunately.
If the aim is discourage use of this common private API and not blocking a feature, please consider deprecating it with advice what to use instead in the message. It will be much less pressure on user/customer support and less likely to negatively affect developer sentiment, especially those with long running projects.
non-forced update
Can you please provide a definition of it? There were no such updates before, so is this different version that users can migrate to, is it a separate version that users use one or the other, will it be forced after some time, what is it?
Many people and projects rely on
pc.app, and it is up to docs to provide meaningful info on when it is not recommended.The reason people use it - is because it is useful and convenient. So there should be a strong reason for its removal, otherwise, it just makes devs lives less convenient and introduces change for no technical reason.
With regards to the forced update I had assumed the community already knew about the split between engine v1 and v2. We will properly outline what this is and how it will work in a later post.
As for pc.app taking @mvaligursky's point into account technically all global access to the current application should be avoided since for multi applications its reference is ambiguous. For single applications however I see the usefulness of having global access to the current application despite it not being the best practise to use.
With regards to the forced update I had assumed the community already knew about the split between engine v1 and v2. We will properly outline what this is and how it will work in a later post.
I read every PR here on git, and am very up-to-date with the engine's development, and this is somewhat surprising to hear. I don't think there was any discussion on such, nor blog post. The only mention was about the LTS-webgl1 version to be supported by the editor.