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

AppBase refactored out of the Application to allow for tree-shaking #4082

Merged
merged 17 commits into from
Apr 5, 2022

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Mar 4, 2022

Based on #3949

This is an initial refactoring of AppBase class out of the Application. There will be follow up PRs removing some additional functionality / imports from AppBase.

The situation before this PR: we have an Application module, which imports and instantiates every other module the application needs - even when the user's application does not need them, limiting the usefulness of tree shaking.

This PR extracts a BaseApp class from the Application, which holds and uses all other instances of classes as before, but does not instantiate them. This part is left in the Application.

This allows the user to instead of using Application with all dependencies, to simply use the AppBase, and decide what dependencies are needed.

As an example, at the moment this is what users do to create application, which imports all modules from the engine:

const Application = new pc.Application();

this PR allows to allocate AppBase, and import only what is needed:

import { AppBase } from './app-base.js';
import { AppCreateOptions } from './app-create-options.js';
import { WebglGraphicsDevice } from '../graphics/webgl/webgl-graphics-device.js';
import { RigidBodyComponentSystem } from './components/rigid-body/system.js';
import { CollisionComponentSystem } from './components/collision/system.js';
import { Lightmapper } from '../scene/lightmapper/lightmapper.js';

const createOptions = new AppCreateOptions();

// user can import and create webgl or webgpu based on the needs
createOptions.graphicsDevice = new WebglGraphicsDevice(canvas, graphicsDeviceOptions);

// import and use only components that are neded
createOptions.componentSystems = [
            RigidBodyComponentSystem,
            CollisionComponentSystem
];
  
// import and use lightmapper only when needed
createOptions.lightmapper = Lightmapper;
      
const app = new pc.AppBase(canvas);
app.init(createOptions);

The way this works is that user imports the module, and assigns a class constructor to the createParameters. This allows AppBase Internally to call new on that object to allocate the instance. This removes the need for the user to call the constructor at all. There are few advantages here:

  • we can internally change the constructor parameters as needed
  • the user does not need to understand parameters to constructors, which can be hidden from docs (as currently)
  • we can call constructors in order we need internally

example for this. We use this:

createOptions.lightmapper = Lightmapper;

instead of

createOptions.lightmapper = new Lightmapper(device, app.root, app.scene, app.renderer, app.assets);

Basic testing to confirm we load less modules (using minimum app)

Screenshot 2022-03-09 at 11 21 45

@mvaligursky mvaligursky changed the title Lite version of the Application to allow for tree-shaking (WIP) AppBase refactored out of the Application to allow for tree-shaking (WIP) Mar 4, 2022
@mvaligursky mvaligursky marked this pull request as draft March 4, 2022 09:42
@LeXXik
Copy link
Contributor

LeXXik commented Mar 4, 2022

It would be great, if we would have access to components selection from the Editor. The builder would then assemble an application based on that configuration.

@mvaligursky
Copy link
Contributor Author

It would be great, if we would have access to components selection from the Editor. The builder would then assemble an application based on that configuration.

Yes, in the longer term the plan is to certainly allow tree shaking in the Editor, likely in a form of some builder. The complexity we need to solve after this PR is the scripts - as those are not modules (for now anyways), and so we cannot easily find out what needs to be imported to the application.

@mvaligursky mvaligursky marked this pull request as ready for review March 9, 2022 09:45
@mvaligursky mvaligursky requested a review from a team March 9, 2022 09:45
@mvaligursky mvaligursky self-assigned this Mar 24, 2022
Martin Valigursky added 4 commits April 5, 2022 09:35
# Conflicts:
#	src/font/canvas-font.js
#	src/resources/container.js
#	src/scene/picker.js
@mvaligursky mvaligursky changed the title AppBase refactored out of the Application to allow for tree-shaking (WIP) AppBase refactored out of the Application to allow for tree-shaking Apr 5, 2022
@mvaligursky mvaligursky merged commit 08b1f08 into main Apr 5, 2022
@mvaligursky mvaligursky deleted the mvaligursky-appbase branch April 5, 2022 14:19
@slimbuck slimbuck mentioned this pull request Jul 27, 2022
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

Successfully merging this pull request may close these issues.

4 participants