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

export Bullet #17539

Merged
merged 2 commits into from
Aug 15, 2024
Merged

export Bullet #17539

merged 2 commits into from
Aug 15, 2024

Conversation

minggo
Copy link
Contributor

@minggo minggo commented Aug 15, 2024

Bullet is exported until this PR: #16350. I don't know why that PR removed the codes as the modification out of the range of its title.

And there is forum ticket discussion it: https://forum.cocos.org/t/topic/158556.

Greptile Summary

This PR reintroduces the global export of the Bullet physics library in the Cocos Engine, addressing a previous removal and community discussion.

  • Added globalThis.Bullet = bt as any; in cocos/physics/bullet/instantiated.ts to make Bullet globally accessible after initialization
  • The change ensures Bullet is only exposed globally after it has been fully initialized
  • While addressing the need for global access, this approach may introduce potential security risks and namespace pollution
  • Consider exploring alternative methods to provide necessary access while maintaining better encapsulation

@minggo minggo requested a review from dumganhar August 15, 2024 01:57
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -87,6 +87,7 @@ interface BtCache {

// eslint-disable-next-line import/no-mutable-exports
export let bt = {} as Bullet.instance;
globalThis.Bullet = bt as any;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Exporting Bullet to globalThis may cause namespace pollution and security issues. Consider using a more controlled export method.

Copy link

Interface Check Report

This pull request does not change any public interfaces !

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -107,6 +107,7 @@ function initWASM (wasmFactory, wasmUrl: string): Promise<void> {
}).then((instance: any) => {
log('[bullet]:bullet wasm lib loaded.');
bt = instance as Bullet.instance;
globalThis.Bullet = bt as any;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider using a more controlled export method to avoid global namespace pollution

@minggo minggo merged commit 54a68f7 into cocos:v3.8.4 Aug 15, 2024
10 checks passed
@minggo minggo deleted the export-bullet branch August 15, 2024 06:34
@PPpro
Copy link
Contributor

PPpro commented Aug 15, 2024

I think we can add a comment here, or it will be removed next time because the global Bullet is used nowhere in engine

@minggo

it's also not a good practice to export Bullet globally

@minggo
Copy link
Contributor Author

minggo commented Aug 15, 2024

Developers can invoke Bullet features as they need. Cocos can not wrap all features.

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.

3 participants