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

fix: gizmo rendering problem #58

Merged
merged 5 commits into from
Dec 20, 2023
Merged

fix: gizmo rendering problem #58

merged 5 commits into from
Dec 20, 2023

Conversation

liuxi150
Copy link
Contributor

@liuxi150 liuxi150 commented Dec 20, 2023

Summary by CodeRabbit

  • Refactor

    • Enhanced handling of multiple wireframe meshes in editor gizmos for improved performance and reliability.
    • Streamlined destruction of wireframe meshes to ensure proper cleanup and resource management.
  • Bug Fixes

    • Fixed an issue where wireframe meshes might not be correctly disposed of, potentially leading to memory leaks.

Copy link
Contributor

coderabbitai bot commented Dec 20, 2023

Walkthrough

The updates focus on enhancing the handling of wireframe meshes within an editor plugin. Multiple meshes can now be managed with a new array property, and mesh destruction has been refined with updated disposal methods. These changes streamline the graphics processing and resource management for a more robust and flexible editor experience.

Changes

File Path Summary
.../gizmo-loader.ts
.../gizmo-vfx-item.ts
Restructured handling of wireframeMesh and introduced wireframeMeshes array property to manage multiple meshes. Updated methods to iterate over meshes and perform operations based on their properties.
.../wireframe.ts Modified destroyWireframeMesh function to use dispose for mesh geometry and updated SharedGeometry class with an override dispose method and additional cleanup operations.

🐇✨
In the code where meshes intertwine,
A rabbit hopped through, refactoring fine.
With every loop and dispose,
The editor's grace rose,
For a wireframe dance, so divine. 🌟🎶

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between e493738 and e51722a.
Files selected for processing (3)
  • plugin-packages/editor-gizmo/src/gizmo-loader.ts (1 hunks)
  • plugin-packages/editor-gizmo/src/gizmo-vfx-item.ts (3 hunks)
  • plugin-packages/editor-gizmo/src/wireframe.ts (2 hunks)
Additional comments: 6
plugin-packages/editor-gizmo/src/gizmo-loader.ts (1)
  • 161-174: The updated logic to handle an array of wireframeMeshes and the fallback to handle a single wireframeMesh is correctly implemented. The check for isDestroyed before calling destroyWireframeMesh and removing the mesh from the render pass is a good practice to prevent unnecessary operations on already destroyed objects.
plugin-packages/editor-gizmo/src/gizmo-vfx-item.ts (3)
  • 99-99: The addition of the wireframeMeshes array property to the GizmoVFXItem class is correctly implemented to store multiple mesh instances.

  • 201-208: The logic in the createModelContent method correctly initializes the wireframeMeshes array and populates it with meshes created for each model in the ms array. This change supports the handling of multiple wireframe meshes.

  • 333-338: The updateRenderData method has been updated to handle multiple wireframe meshes. The loop correctly iterates over the wireframeMeshes array and updates each mesh using the updateWireframeMesh function. Ensure that the length of meshes and wireframeMeshes arrays are always synchronized to prevent index out-of-bounds errors.

plugin-packages/editor-gizmo/src/wireframe.ts (2)
  • 51-52: The update to the destroyWireframeMesh function to use dispose instead of destroy on mesh.geometry is correct and ensures proper resource cleanup. The mesh.dispose call is also correctly updated to maintain textures while disposing of other materials and geometries.

  • 247-253: The SharedGeometry class has been updated with an override dispose method that includes additional cleanup logic for buffers and bufferProps. The base dispose method is correctly called, and the source property is set to null post-disposal. The use of @ts-expect-error is appropriate if the TypeScript error is expected and understood.

yiiqii
yiiqii previously approved these changes Dec 20, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between e51722a and bb6111b.
Files ignored due to filter (10)
  • packages/effects-core/package.json
  • packages/effects-helper/package.json
  • packages/effects-threejs/package.json
  • packages/effects-webgl/package.json
  • packages/effects/package.json
  • plugin-packages/alipay-downgrade/package.json
  • plugin-packages/editor-gizmo/package.json
  • plugin-packages/model/package.json
  • plugin-packages/orientation-transformer/package.json
  • plugin-packages/spine/package.json
Files selected for processing (1)
  • plugin-packages/editor-gizmo/src/wireframe.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • plugin-packages/editor-gizmo/src/wireframe.ts

RGCHN
RGCHN previously approved these changes Dec 20, 2023
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yaml

Commits Files that changed from the base of the PR and between bb6111b and 9e72d5c.
Files selected for processing (1)
  • plugin-packages/editor-gizmo/src/wireframe.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • plugin-packages/editor-gizmo/src/wireframe.ts

@liuxi150 liuxi150 merged commit d3b75b4 into dev Dec 20, 2023
2 checks passed
@yiiqii yiiqii deleted the fix/gizmo-wireframe branch December 20, 2023 06: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.

5 participants