jmonkeyengine icon indicating copy to clipboard operation
jmonkeyengine copied to clipboard

Refactor: LightsDebugState optimization, javadoc, performance

Open capdevon opened this issue 8 months ago • 9 comments

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.

image image image

capdevon avatar May 30 '25 18:05 capdevon

🖼️ 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

github-actions[bot] avatar Jun 01 '25 10:06 github-actions[bot]

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.

capdevon avatar Jun 03 '25 08:06 capdevon

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.

yaRnMcDonuts avatar Jun 03 '25 08:06 yaRnMcDonuts

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.

capdevon avatar Jun 03 '25 09:06 capdevon

viewPort.setClearFlags(false, true, true);

I think you should add this, but also add an option to enable/disable depth clearing.

codex128 avatar Jun 03 '25 13:06 codex128

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:

  1. add LightsDebugState.getViewPort() method or
  2. 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 image

e.g. clearDepth=false image

capdevon avatar Jun 04 '25 09:06 capdevon

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.

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 viewport directly from the renderManager like 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.

yaRnMcDonuts avatar Jun 04 '25 11:06 yaRnMcDonuts

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.

codex128 avatar Jun 04 '25 11:06 codex128

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.

capdevon avatar Jun 04 '25 13:06 capdevon