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

Feature: AR Viewer #56

Merged
merged 77 commits into from
Dec 16, 2024
Merged

Feature: AR Viewer #56

merged 77 commits into from
Dec 16, 2024

Conversation

ffrank913
Copy link
Contributor

1. Why is this change necessary?

2. What does this change do, exactly?

3. Describe each step to reproduce the issue or behaviour.

4. Please link to the relevant issues (if any).

5. Checklist

  • I have rebased my changes to remove merge conflicts
  • I have written tests and verified that they fail without my change
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.

@ffrank913 ffrank913 marked this pull request as ready for review December 5, 2024 13:18
@dfrancos-hub
Copy link
Contributor

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Dec 5, 2024

PR Reviewer Guide 🔍

(Review updated until commit aecf910)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Bug
The onTouchMove method logs the arHitPosition to the console, which might be unnecessary in production. Consider removing or wrapping it in a debug flag.

Code Smell
The Dispose method in DIVEWebXRRaycaster does not clean up all resources, such as event listeners or buffers. Ensure proper cleanup to avoid memory leaks.

Error Handling
The findSceneViewerSrc method throws an error if no model is found. Consider providing a more user-friendly error message or fallback behavior.

Performance Issue
The extractModels method directly accesses scene.Root.children. This could lead to unintended side effects if the children array is modified elsewhere. Consider cloning the array.

Possible Bug
The _onSessionEnded method resets camera values to zero, which might not be the intended behavior. Verify if this is correct or if it should restore previous camera positions.

src/dive.ts Outdated Show resolved Hide resolved
@ffrank913 ffrank913 merged commit eec140a into trunk Dec 16, 2024
7 checks passed
@ffrank913 ffrank913 deleted the feature/ar-viewer branch December 16, 2024 12:28
@CodiumAI-Agent
Copy link

Persistent review updated to latest commit aecf910

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.

4 participants