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

Bug with custom build: Circular Dependency Causes Actor to be undefined in ParticleEmitter #3055

Closed
JumpLink opened this issue May 10, 2024 · 1 comment · Fixed by #3056
Closed
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior

Comments

@JumpLink
Copy link
Contributor

JumpLink commented May 10, 2024

Description

I am using TypeScript source files directly from Excalibur.js instead of the bundle for several advantages:

  • Better source maps as my project directly uses the sources, it can generate better source maps.
  • I can decide for myself which target (e.g. ESNext, ESM, ...) I want to use
  • Ability to make changes without needing to rebuild Excalibur with fast page reload.
  • Direct access to the source code in my IDE.
  • Faster builds with esbuild (or possibly vite, which also utilizes esbuild) which is significantly faster than Excalibur’s webpack.

This approach has been working well until a recent issue where Actor is undefined when referenced in the ParticleEmitter class definition. This seems to be due to circular dependencies and import order issues, causing Actor to be undefined when ParticleEmitter tries to extend it in my own bundled file.

I understand if this is not recognised as a bug as this is a very specific problem, I am happy to fix it myself and hope it is accepted.

Steps to Reproduce

Build Excalibur with Vite or ESBuild with a small sample game, this needs configurations so that this also works with the imported *.css and .glsl files because vite needs to import them using a ?raw prefix by default like this:

import lineVertexSource from './line-vertex.glsl?raw';

I can create an working example using esbuild or vite if there is a need for it.

Expected Result

ParticleEmitter should successfully extend Actor without encountering an undefined issue.

Actual Result

The Actor class is undefined when ParticleEmitter attempts to extend it, due to circular dependencies and import order issues within the TypeScript source files.

Environment

  • Excalibur versions: latest main branch
  • Build tools: esbuild/vite (latest versions)

Current Workaround

Just use the official bundle instead of the sources directly as intended.

@eonarheim eonarheim added the bug This issue describes undesirable, incorrect, or unexpected behavior label May 10, 2024
@eonarheim
Copy link
Member

@JumpLink thanks for the fix, I'll gladly accept this one! This actually bit me in the wallaby config and I didn't dig into the cause.

We'll probably stick to webpack for the core repo for a while because of the control it gives over compilation and the ability to pull in non-js files into the bundle easily. However, I totally understand the desire for increase speed when iterating like you are in a game (I use vite/parcel when building games usually).

In other news I'm working on GPU based particles for the next version 🤞 hopefully I get them completed soon! https://eonarheim.github.io/webgl-particles/ We should be able to render very large numbers of particles and have them collide with a mask!

eonarheim pushed a commit that referenced this issue May 10, 2024
This PR resolves a circular dependency issue in Excalibur.js when using TypeScript source files directly. It refactors `ParticleEmitter` by isolating it into a separate file and adjusting the dependency management in `Particles.ts`.

This PR has the advantage that you should not encounter this problem if Webpack should ever be replaced.

Closes  #3055

## Changes
* Isolated `ParticleEmitter`: Moved `ParticleEmitter` to its own file to manage dependencies more effectively.
* Updated `Particles.ts`: Replaced direct dependency with a type import of `ParticleEmitter`.
* Instance Check Adjustment: Replaced the `instanceof` check with a `constructor` presence check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants