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

🔫 Remove autoUpdate option #22

Merged
merged 1 commit into from
Dec 29, 2021
Merged

🔫 Remove autoUpdate option #22

merged 1 commit into from
Dec 29, 2021

Conversation

marcofugaro
Copy link
Member

@marcofugaro marcofugaro commented Dec 28, 2021

Fixes #21

Having an internal raf loop has resulted in being problematic since the wireframe meshes were not in sync with the bodies. Also this behaviour appears to be confusing for cannon.js users.

Moreover, not even use-cannon is actually using this feature.

Makes sense to remove it completely.

I also went ahead and updated the signature to match the original CannonDebugRenderer.

@marcofugaro
Copy link
Member Author

Will release 1.0 after this

@marcofugaro marcofugaro merged commit 3ec38f9 into master Dec 29, 2021
@marcofugaro marcofugaro deleted the remove-internal-loop branch December 29, 2021 10:20
@@ -240,7 +239,7 @@ export default function cannonDebugger(

let meshIndex = 0

for (const body of bodies) {
for (const body of world.bodies) {
Copy link
Member

@bjornstar bjornstar Dec 31, 2021

Choose a reason for hiding this comment

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

Considering that you only use the world to access the bodies, it seems a bit overkill to bring the entire world into the debugger.

In use-cannon the world lives in a worker and we don't have direct access to it so it's quite difficult to have this as a requirement for using the debugger.

Copy link
Member Author

Choose a reason for hiding this comment

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

I preferred matching the signature to the original CannonDebugRenderer.

We can just mock the world (like we're doing now) in use-cannon.

@@ -30,13 +30,12 @@ export type DebugOptions = {
scale?: number
onInit?: (body: Body, mesh: Mesh, shape: Shape) => void
onUpdate?: (body: Body, mesh: Mesh, shape: Shape) => void
autoUpdate?: Boolean
}

export default function cannonDebugger(
Copy link
Member

Choose a reason for hiding this comment

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

This does not match what is in the README:

export default class CannonDebugger {
  constructor(scene: Scene, world: World, options: DebugOptions): void
  update(): void
}

Copy link
Member Author

Choose a reason for hiding this comment

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

we're using this pattern because we don't want to expose the private variables, we can switch to a class when we can ship private class fields

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.

PERFORMANCE WARNING!!! After migrated from cannon.js to cannon-es-debugger.
2 participants