Fix specular half vector for water reflections #6
Conversation
Summary of ChangesHello @clockcrockwork, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the visual fidelity and interactivity of the moon reflection experience by introducing dynamic moon phase simulation. It allows the moon's appearance and environmental lighting to change from new moon to full moon, while carefully separating moon-driven visual effects from user interaction feedback on the water surface. This ensures that the interactive experience remains consistent and visible regardless of the moon phase. Additionally, a critical bug in the water shader's specular half-vector calculation has been fixed, leading to more accurate and stable reflections. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic moon phase system, complete with a UI slider for debugging, and significantly refactors the lighting and shader effects. The core improvement is the correction of the specular half-vector calculation in the water shader, which provides much more stable and realistic highlights. The changes are well-aligned with the new design document, which smartly decouples interaction feedback from the world's lighting state (moon phase) to preserve the user experience.
My review includes a few suggestions to improve code clarity and maintainability by removing redundant code and replacing magic numbers with named constants. Overall, this is an excellent update that greatly enhances the visual and interactive quality of the project.
| const synodicMonth = 29.53058867; | ||
| const knownNewMoon = Date.UTC(2000, 0, 6, 18, 14, 0); | ||
| const daysSinceKnown = (date.getTime() - knownNewMoon) / 86400000; |
There was a problem hiding this comment.
The magic numbers 29.53058867 and 86400000 are used directly. Defining them as named constants like SYNODIC_MONTH_DAYS or MILLISECONDS_PER_DAY would make the code's intent clearer and improve maintainability.
| blending: THREE.AdditiveBlending, | ||
| depthWrite: false | ||
| depthWrite: false, | ||
| side: THREE.DoubleSide |
There was a problem hiding this comment.
moonGlow is implemented as a billboard that always faces the camera via moonGlow.quaternion.copy(camera.quaternion) in the animate function. Because of this, the back side of the mesh will never be visible, making side: THREE.DoubleSide unnecessary. Removing it could offer a minor performance improvement and make the code's intent clearer.
| side: THREE.DoubleSide | ||
| }) | ||
| ); | ||
| moonGlow.rotation.x = -Math.PI / 2; |
There was a problem hiding this comment.
This moonGlow.rotation.x setting has no effect because it is overwritten every frame by moonGlow.quaternion.copy(camera.quaternion) in the animate function (line 677). This line should be removed to avoid confusion.
| starfield.material.uniforms.time.value = time; | ||
| water.material.uniforms.time.value = time; | ||
|
|
||
| touchEnergy *= 0.92; |
There was a problem hiding this comment.
The magic number 0.92 is used as the decay factor for touchEnergy. Similarly, line 580 uses 0.6 for energy gain. Defining these values in the CONFIG object, for instance as touchEnergyDecay and touchEnergyGain, would make them easier to tweak and improve code readability.
There was a problem hiding this comment.
Pull request overview
This pull request implements dynamic moon phase calculation and updates the water reflection shader to provide phase-aware visual effects while preserving interaction feedback. The core change corrects the specular half-vector calculation for more stable water reflections, and introduces a moon phase system that varies lighting, star density, and glow effects based on the lunar cycle.
Key changes:
- Corrected water shader's specular half-vector from subtraction to addition (Blinn-Phong formula)
- Added dynamic moon phase calculation with UI controls for development/testing
- Implemented decoupled lighting system that maintains interaction feedback regardless of moon phase
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| moon_phase_design.md | Comprehensive design document (in Japanese) explaining the philosophy and technical approach for moon phase implementation while preserving tactile water interaction experience |
| index.html | Core implementation: moon phase calculation, phase-aware shader updates, DirectionalLight for moon illumination, star density modulation, touch energy system, and UI controls |
| README.md | Added reference link to the new design document |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| side: THREE.DoubleSide | ||
| }) | ||
| ); | ||
| moonGlow.rotation.x = -Math.PI / 2; |
There was a problem hiding this comment.
The initial rotation setting moonGlow.rotation.x = -Math.PI / 2 at line 332 will be immediately overwritten by moonGlow.quaternion.copy(camera.quaternion) at line 677 in the first animation frame. Consider removing this initial rotation setting as it serves no purpose and could be misleading to future maintainers.
| moonGlow.rotation.x = -Math.PI / 2; |
Summary
Testing
Codex Task