-
Notifications
You must be signed in to change notification settings - Fork 132
Bundle PlayCanvas Engine into Editor #1674
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modernizes the editor by bundling the PlayCanvas engine directly into editor.js using ES module imports, removing the dependency on a globally injected pc namespace. This improves tree-shaking potential and aligns the codebase with modern module practices while maintaining backward compatibility for plugins through global exposure.
Key Changes:
- Bundle
playcanvasintoeditor.jswhile keeping it external forlaunch.js - Add ES module imports across ~40 files replacing global
pc.*access - Update deprecated shadow constants to their modern equivalents
Reviewed changes
Copilot reviewed 70 out of 71 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/editor/pc-global.ts | New file exposing engine API globally for plugins |
| rollup.config.mjs | Configure playcanvas as external for launch.js only |
| types.d.ts | Update comment about pc loading |
| src/core/constants.ts | Re-export engine constants, remove duplicates |
| src/common/utils.ts | Fix validateEnginePath to use in operator |
| src/editor/viewport/*.ts | Convert to ES module imports |
| src/editor/viewport/gizmo/*.ts | Convert to ES module imports |
| src/editor/viewport/camera/*.ts | Convert to ES module imports |
| src/editor/inspector/components/*.ts | Update deprecated shadow constants |
| src/editor/inspector/assets/*.ts | Replace pc.app with editor.call |
| src/editor/templates/*.ts | Import guid from playcanvas |
| src/editor/entities/*.ts | Import engine classes |
| src/editor/assets/*.ts | Import engine classes |
| src/common/*.ts | Import engine classes |
| src/editor/index.ts | Import pc-global early |
| src/editor/blank.ts | Import pc-global early |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import * as pc from 'playcanvas'; | ||
|
|
||
| // Expose engine API globally for plugins/extensions | ||
| (window as any).pc = pc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typing should explicitly extend the window interface and not cast as any
| // pc (bundled in editor, injected in HTML for launch page) | ||
| declare var pc: typeof import('playcanvas'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be removed and scoped to just the launcher
| let obj: any = pc; | ||
| for (let i = 0; i < parts.length; i++) { | ||
| if (!obj.hasOwnProperty(parts[i]) && obj[parts[i]] === undefined) { | ||
| if (!(parts[i] in obj) && obj[parts[i]] === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should try not casting to any - use exact typing since we have it available
| export { | ||
| // Layer IDs | ||
| LAYERID_WORLD, | ||
| LAYERID_DEPTH, | ||
| LAYERID_SKYBOX, | ||
| LAYERID_IMMEDIATE, | ||
| LAYERID_UI, | ||
| // Gamma correction modes | ||
| GAMMA_NONE, | ||
| GAMMA_SRGB, | ||
| // Curve types | ||
| CURVE_LINEAR, | ||
| CURVE_SMOOTHSTEP, | ||
| CURVE_SPLINE, | ||
| CURVE_STEP, | ||
| // Anim constants | ||
| ANIM_INTERRUPTION_NONE, | ||
| ANIM_INTERRUPTION_PREV, | ||
| ANIM_INTERRUPTION_NEXT, | ||
| ANIM_INTERRUPTION_PREV_NEXT, | ||
| ANIM_INTERRUPTION_NEXT_PREV, | ||
| ANIM_GREATER_THAN, | ||
| ANIM_LESS_THAN, | ||
| ANIM_GREATER_THAN_EQUAL_TO, | ||
| ANIM_LESS_THAN_EQUAL_TO, | ||
| ANIM_EQUAL_TO, | ||
| ANIM_NOT_EQUAL_TO, | ||
| ANIM_PARAMETER_INTEGER, | ||
| ANIM_PARAMETER_FLOAT, | ||
| ANIM_PARAMETER_BOOLEAN, | ||
| ANIM_PARAMETER_TRIGGER | ||
| } from 'playcanvas'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to rexport - just use the values directly. Any backwards compatible constants should be explicitly defined in this file
Fixes #1457
This PR bundles the PlayCanvas engine directly into
editor.jsusing ES module imports instead of relying on a globally injectedpcnamespace. This improves tree-shaking potential (although we will probably need to retain the whole engine to be bundled for plugin writers) and aligns the editor with modern module practices.One very nice benefit is that we now get full Intellisense in VS Code for the Engine API.
Changes
Build Configuration
rollup.config.mjsto bundleplaycanvasintoeditor.js(removed from externals)launch.jscontinues to use an externally injected engineGlobal Engine Exposure
src/editor/pc-global.tsto expose the engine API aswindow.pcfor editor extensions/pluginsindex.tsandblank.tsentry pointsES Module Migration
pc.*access to explicit ES module imports fromplaycanvasConstants Cleanup
src/core/constants.tsLAYERID_*,GAMMA_*,CURVE_*, andANIM_*constants fromplaycanvasDeprecated API Updates
SHADOW_VSM8→SHADOW_VSM_16FSHADOW_VSM32→SHADOW_VSM_32FSHADOW_PCF1→SHADOW_PCF1_32FSHADOW_PCF3→SHADOW_PCF3_32FSHADOW_PCF5→SHADOW_PCF5_32FBug Fixes
pc.app = appassignment (ES module namespaces are immutable; engine sets this internally)validateEnginePathto useinoperator instead ofhasOwnProperty(ES module namespace objects have null prototype)Notes
UMD format retained for
editor.js- ESM migration can follow in a subsequent PRThe engine is automatically set via
AppBaseconstructor, so manualpc.appassignment was redundantI confirm I have read the contributing guidelines