add documentation and example for custom controller#568
add documentation and example for custom controller#568RaulPROP wants to merge 1 commit intovasturiano:masterfrom
Conversation
micahstubbs
left a comment
There was a problem hiding this comment.
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
- Debug code: Line 119 in
controller.jscontainsconsole.log("DEBUG keydown", event);which should be removed from production code - Missing TypeScript definitions: The
src/index.d.tsfile needs to be updated to reflect thatcontrolTypecan now accept a function type:controlType?: 'trackball' | 'orbit' | 'fly' | ((camera: Camera, domElement: HTMLElement) => any)
📝 Documentation Improvements
- Function signature: The README should specify the expected function signature more clearly:
controlType: (camera, domElement) => Controller - 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
- Code organization: The controller.js file is quite large (314 lines). Consider adding some inline comments explaining key sections
- 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.
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.