Skip to content

Conversation

@capdevon
Copy link
Contributor

@capdevon capdevon commented Jun 2, 2025

Ensures consistent and informative debug output across all light types.
Adds missing toString() implementations for AmbientLight, PointLight and SpotLight, and refines existing ones for DirectionalLight and ProbeLight for better debug output.

@capdevon capdevon changed the title Title: Refine Light toString() Methods Consistent toString() for Lights Jun 2, 2025
@yaRnMcDonuts yaRnMcDonuts added this to the v3.9.0 milestone Jun 3, 2025
@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Jun 8, 2025

I notice this PR includes changed return values for some existing toString() methods, which I had also mentioned being worried about in another PR.

The chance of this breaking something for someone is definitely low, although still possible.

But I still honestly am not sure if this is something important to worry about or if I'm being over cautious. In my own personal code I'd change it without worrying much, but from what I'm googling it sounds like a toString() method's return values should avoid being changed unless necessary for public APIs like jme. But maybe it would be useful to get a core dev to comment and give their opinion on this since I'm just speculating and being overly cautious.

@yaRnMcDonuts
Copy link
Member

So I think this PR should be okay now after taking another look and considering it more.

Adding the missing toString() methods for the the light classes that don't have it is definitely useful.

The only thing I was hesitant about was the change in format for the string that was returned by the existing method in LightProbe.

I'd typically say to keep with the existing format for all lights, just to minimize changes. But in this case it already looks like DirectionalLight and LightProbe had inconsistent formats in their existing toString() methods . And regardless of which formatting is better, the important part is probably just to be consistent with all lights, which looks to be the case now.

So I'd say this PR looks good to me now.

@yaRnMcDonuts yaRnMcDonuts merged commit 1244723 into jMonkeyEngine:master Jun 25, 2025
16 checks passed
@capdevon capdevon deleted the capdevon-Light-toString branch June 30, 2025 10:10
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