Fix cosine calculation and improve pdf stability in Emissive light im…

[doubleailes]

User description

…plementation

---

PR Type

Bug fix, Enhancement

---

Description

• Fixed cosine calculation in pdf method for accuracy.

• Improved numerical stability by adjusting denominator epsilon.

• Enhanced light sampling by correcting geometric relationships.

---

Changes walkthrough 📝

	Relevant files
Bug fix	emissive.rs Fix and enhance PDF calculation in Emissive material          crates/crust-render/src/material/emissive.rs Corrected cosine calculation in the pdf method. Adjusted epsilon value in denominator for stability. Improved light sampling by refining geometric relationships. +2/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 74 - Fully compliant Compliant requirements: • Fix the PDF implementation to properly use the input parameters _hit_point and _light_point • Account for the geometric relationship between these points for physically correct rendering

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Cosine Calculation The cosine calculation now uses the normal vector from the light point to the light position, rather than from the light point to the hit point. Verify this is the correct geometric relationship for this specific light model. let cosine: f32 = f32::max(normal.dot((light_point - self.position).normalize()), 0.0); let area = 4.0 * std::f32::consts::PI * self.radius * self.radius; Epsilon Value The epsilon value in the denominator was reduced from 1e-4 to 1e-8. Verify this doesn't introduce numerical instability in edge cases with very small cosine values. distance_squared / (cosine * area + 1e-8) }

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

crates/crust-render/src/material/emissive.rs:81

• Reducing the epsilon from 1e-4 to 1e-8 could potentially lead to numerical instability when cosine values are near zero. Please review this change to ensure that it maintains adequate protection against division by very small numbers.

distance_squared / (cosine * area + 1e-8)

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Handle near-zero cosine values While reducing the epsilon from 1e-4 to 1e-8 might improve precision, it could lead to potential floating-point instability in extreme cases. Consider adding a check to ensure cosine isn't too close to zero before division. crates/crust-render/src/material/emissive.rs [81] -distance_squared / (cosine * area + 1e-8) +if cosine < 1e-6 { + f32::MAX // Or another appropriate value for when rays are nearly parallel to the surface +} else { + distance_squared / (cosine * area + 1e-8) +} Apply this suggestion Suggestion importance[1-10]: 8 __ Why: The suggestion addresses a potential numerical instability issue when cosine values approach zero. Adding an explicit check prevents division by very small numbers that could lead to floating-point errors or unexpected behavior in edge cases.	Medium
Possible issue	✅ Fix inconsistent normal calculation Suggestion Impact: The commit addresses the inconsistency in the normal calculation, but instead of renaming the variable, it fixes the actual calculation by changing (light_point - self.position) to (light_point - hit_point), which makes the calculation consistent with the normal definition code diff: - let cosine: f32 = f32::max(normal.dot((light_point - self.position).normalize()), 0.0); + let cosine: f32 = f32::max(normal.dot((light_point - hit_point).normalize()), 0.0); The normal calculation is incorrect. You're using the normalized direction from hit_point to light_point as the normal, but then comparing it with a different vector. For consistency, either use the surface normal of the light or simplify the calculation. crates/crust-render/src/material/emissive.rs [78-79] -let normal = direction.normalize(); -let cosine: f32 = f32::max(normal.dot((light_point - self.position).normalize()), 0.0); +let direction_normalized = direction.normalize(); +let cosine: f32 = f32::max(direction_normalized.dot((light_point - self.position).normalize()), 0.0); [Suggestion has been applied] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a naming inconsistency between "normal" and its actual use as a direction vector. Renaming to "direction_normalized" improves code clarity and prevents potential confusion about what the variable represents.	Medium
Update