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

Feat/simplify three usage #32

Merged
merged 2 commits into from
May 27, 2024
Merged

Feat/simplify three usage #32

merged 2 commits into from
May 27, 2024

Conversation

piellardj
Copy link
Member

This PR:

  • fixes an issue where the aresrpg-engine sometimes imported three directly instead of using the centralized three-usage.ts
  • tries to fix a bug where at the start up of the aresrpg-dapp, we see 2 warnings
    image
    These warnings mean that aresrpg-world, aresrpg-dapp and aresrpg-engine had different THREE versions.
    My understanding is that it happens because aresrpg-engine wants a THREE v0.163.0 whereas aresrpg-dapp wants a THREE v0.164.1, so NPM installs both versions and both are bundled in the final product. To fix this, we could always synchronize versions in the 3 packages, but it is not very easy to do. I hope passing three as a peerDependecy instead of a dependency will make npm understand that aresrpg-engine can use the same version as aresrpg-dapp.
    Importing multiple THREE versions is bad because:
    • it might lead to bugs
    • it slows down the start up of aresrpg-dapp, since a minified THREE is abound 600KB of minified javascript, so loading 3 different versions takes network and CPU time uselessly

This is an attempt to fix the "Multiple instances of Three.js in same page" error in aresrpg-dapp which uses its own THREEjs and aresrpg-engine.
@piellardj piellardj requested a review from Sceat May 27, 2024 11:34
@piellardj piellardj self-assigned this May 27, 2024
@Sceat Sceat merged commit 30b990b into master May 27, 2024
2 checks passed
@piellardj piellardj deleted the feat/simplify_three_usage branch May 28, 2024 16:03
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.

2 participants