Refactor: LightsDebugState optimization, javadoc, performance
This PR significantly enhances the LightsDebugState class by:
- Optimizing performance: Reduced overhead for more efficient debugging.
- Fixing bugs: Addressed several issues for improved stability.
- Improving resource management: Ensured proper cleanup and allocation.
- Adding Javadoc: Provided comprehensive documentation for better understanding and maintainability.
-
Unified Light Visualization:
LightProbe,DirectionalLight,PointLight&SpotLight. - Dynamic Scene Integration: The state automatically detects and creates/updates gizmos for lights present in the configured scene graph. Includes logic to clean up gizmos for lights that have been removed from the scene, ensuring the debug overlay remains accurate and clutter-free.
Edit: This PR is still work in progress, and is not yet ready to be merged...
Edit: Ready!
This comprehensive LightsDebugState streamlines the process of debugging lighting setups, making it easier to visualize light behavior and troubleshoot common lighting issues in JME applications.
🖼️ Screenshot tests have failed.
The purpose of these tests is to ensure that changes introduced in this PR don't break visual features. They are visual unit tests.
📄 Where to find the report:
- Go to the (failed run) > Summary > Artifacts > screenshot-test-report
- Download the zip and open jme3-screenshot-tests/build/reports/ScreenshotDiffReport.html
⚠️ If you didn't expect to change anything visual: Fix your changes so the screenshot tests pass.
✅ If you did mean to change things: Review the replacement images in jme3-screenshot-tests/build/changed-images to make sure they really are improvements and then replace and commit the replacement images at jme3-screenshot-tests/src/test/resources.
✨ If you are creating entirely new tests: Find the new images in jme3-screenshot-tests/build/changed-images and commit the new images at jme3-screenshot-tests/src/test/resources.
Note; it is very important that the committed reference images are created on the build pipeline, locally created images are not reliable. Similarly tests will fail locally but you can look at the report to check they are "visually similar".
See https://github.com/jMonkeyEngine/jmonkeyengine/blob/master/jme3-screenshot-tests/README.md for more information
Feat: Add light filter to customize debug light display
Introduced a lightFilter parameter and its corresponding setter method (setLightFilter) to allow for selective display of lights in the debug state. This enables users to define a Predicate<Light> to control which lights are visualized, improving debugging flexibility.
Looking forward to testing this out in my own custom editor soon! My editor's light tool is pretty bare bones and lacks any visualization (my lights are just represented by empty invisible nodes that can be moved around) so this will be very useful in my editor for improving my light tool, and should be very useful for other jme users' custom editors. Thanks for working on this class!
I may have more feedback once I've used the class, but so far everything looks good to me,. So just let me know whenever you're all done and ready for it to be merged.
I am undecided about whether or not to add this instruction:
viewPort.setClearFlags(false, true, true);
Otherwise, I think the class is ready to be tested.
viewPort.setClearFlags(false, true, true);
I think you should add this, but also add an option to enable/disable depth clearing.
Configuring the clearDepth parameter has turned out to be more complex than anticipated, and the visual results aren't consistently clear. I explored two options:
- add
LightsDebugState.getViewPort()method or - add
LightsDebugState.setClearDepthViewPort(boolean)method.
The main hurdle is that the viewport is only initialized within LightsDebugState.initialize(). Introducing these methods would create scenarios where they're invoked on a null viewport, leading to confusion and the need for additional error handling.
Therefore, my conclusion is that the most sensible approach is to forego adding any new methods and maintain the default viewPort flags. The reasoning is that enabling clearDepth=true by default would cause all debug geometries and light icons to always render in the foreground, which would likely be more disorienting than helpful.
For those who need to customize the viewport flags, the flexibility remains:
you can always retrieve the viewport directly from the renderManager like this:
ViewPort vp = renderManager.getMainView("EnvDebugView");
vp.setClearDepth(true);
Edit:
That is why I would rename the viewPort to either LightsDebugView or LightGizmosView.
e.g. clearDepth=true
e.g. clearDepth=false
The reasoning is that enabling
clearDepth=trueby default would cause all debug geometries and light icons to always render in the foreground, which would likely be more disorienting than helpful.
On one hand I agree that setting true makes it disorienting and clutters the screen in small scenes with many lights. like the test scene you are using in your screenshots.
But on the other hand, setting it false will also cause the debug circles and light gizmos to be easily lost and hard to find when viewing very large scenes, especially at a distance and especially if the light is under tree canopies or inside a building.
I know that I will have an option to toggle this in my editor's light tool when I use this class, since an editor can get cluttered when enabling extra detail, but you also still want to be able to have the option to toggle that extra detail when the task requires it.
For those who need to customize the viewport flags, the flexibility remains: you can always retrieve the
viewportdirectly from therenderManagerlike this:ViewPort vp = renderManager.getMainView("EnvDebugView"); vp.setClearDepth(true);
I'd say that putting this code in a simple convenience method in the LightsDebugState is the best option, because otherwise many users will not find this information to know how to change this, especially newer users who don't fully understand how this class works using view ports and depth writing. Ideally the class should be as convenient and easy to use to its full potential without the typical user having to delve too far into the source code or understand too much of the class' deeper functionality.
So I'd agree with @codex128 that making a convenience method with that code in it is best. Something straightforward like setShowLightsDebugOverScene(boolean showOverScene) or something similar would be a good name in my opinion.
The main hurdle is that the viewport is only initialized within LightsDebugState.initialize().
You could add a boolean that sets the depth flag during initialization. I realize it's not very convenient, but it'll work.
Ok, I will make the requested changes. Should the default value of the clearDepth flag be true or false?
Edit: It's currently set to true. Let me know if I need to change it to false.