Skip to content

Conversation

@RaulPROP
Copy link
Contributor

Following this PR, I adapted the README and added an example (same as in thee-render-objects) to show the use of a custom controller.

Copy link
Contributor

@micahstubbs micahstubbs left a comment

Choose a reason for hiding this comment

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

Overall Assessment

This PR successfully adds support for custom camera controllers by allowing controlType to accept a constructor function, which is a valuable enhancement to the library's flexibility. The implementation follows the upstream changes from three-render-objects and provides a practical example.

Positive Aspects

Good Documentation: The README update clearly explains the new functionality and maintains consistency with existing documentation format
Practical Example: The FirstPersonControls example demonstrates real-world usage and provides comprehensive camera movement
Consistent Pattern: Uses the same constructor function approach as other examples (new ForceGraph3D())
Follows Upstream: Properly implements the three-render-objects functionality

Issues & Suggestions

🔧 Code Quality

  1. Debug code: Line 119 in controller.js contains console.log("DEBUG keydown", event); which should be removed from production code
  2. Missing TypeScript definitions: The src/index.d.ts file needs to be updated to reflect that controlType can now accept a function type:
    controlType?: 'trackball' | 'orbit' | 'fly' | ((camera: Camera, domElement: HTMLElement) => any)

📝 Documentation Improvements

  1. Function signature: The README should specify the expected function signature more clearly:
    controlType: (camera, domElement) => Controller
    
  2. Example consistency: Consider using CDN instead of local dist in the example for consistency with other examples (or use commented approach like others)

🎯 Example Enhancements

  1. Code organization: The controller.js file is quite large (314 lines). Consider adding some inline comments explaining key sections
  2. Error handling: The example could benefit from basic error handling for the controller initialization

Minor Suggestions

  • The example works well but could include brief instructions about the key bindings (WASD, RF for up/down, mouse for look)
  • Consider adding the example to the main examples list in the correct alphabetical order

Conclusion

This is a solid contribution that adds important functionality. The main blocker is the debug console.log that should be removed, and the TypeScript definitions should be updated. Once those are addressed, this would be ready to merge.

Recommendation: Request changes for the debug code removal and TypeScript definition update, then approve.

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