-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Implement an Sample-Distribution Shadow Mapping filter #2578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
codex128
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry this took me so long.
| } | ||
|
|
||
| //TODO: How should the GL4 specific functionalities here be exposed? Via the renderer? | ||
| public GL4 getGl4(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently there is no good way to expose gl4, so this will be acceptable for now.
| gl.glActiveTexture(GL.GL_TEXTURE0 + bindingPoint); | ||
| int textureId = texture.getImage().getId(); | ||
| int target = convertTextureType(texture); | ||
| gl.glBindTexture(target, textureId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will mess up GLRenderer's internal notion of which textures are currently bound. You may want to add a method to Renderer to bind a texture at a point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turned out I can use the regular Renderer method here.
| * The shader should not be used after calling this method. | ||
| */ | ||
| public void delete() { | ||
| gl.glDeleteProgram(programId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest making use of the NativeObjectManager to ensure the shader is properly deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a minor problem with using NativeObjectManager; the renderer doesn't expose its instance. It could be made to expose that (or a way to add NativeObjects to its own?).
I could also create a NativeObjectManager for the filter itself, but it would also need to be polled for dead objects every frame. Not sure what the best way to do it would be.
| * Deletes this buffer and releases GPU resources. | ||
| * The buffer should not be used after calling this method. | ||
| */ | ||
| public void delete() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, consider using NativeObjectManager for this.
| ShaderStorageBufferObject minMaxDepthSsbo; | ||
| ShaderStorageBufferObject fitFrustumSsbo; | ||
| FitParameters parameters; | ||
| long fence = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a dedicated fence object to handle this value, especially if you want to use NativeObjectManager.
|
|
||
| private static String loadShaderSource(AssetManager assetManager, String resourcePath) { | ||
| //TODO: Should these shaders get special loaders or something? | ||
| AssetInfo info = assetManager.locateAsset(new AssetKey<>(resourcePath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make a dedicated asset loader for compute shaders.
public class ComputeLoader implements AssetLoader {
public Object load(AssetInfo assetInfo) {
try (InputStream stream = assetInfo.openStream()) {
// load code from stream
}
}
}
// register the loader to load .comp files
assetManager.registerLoader(ComputeLoader.class, "comp");You could also try using GLSLLoader. It does a lot of preprocessing that might be handy. I don't know if it can be adapted to compute shaders though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the regular GLSLLoader actually just returns a string, so it can easily load there. (Regular shaders also have a bunch of extra code somewhere that turns them into something usable for the renderer, most of which I think doesn't apply to compute shaders.)
| uniform mat4 m_LightViewProjectionMatrix0; | ||
| uniform mat4 m_LightViewProjectionMatrix1; | ||
| uniform mat4 m_LightViewProjectionMatrix2; | ||
| uniform mat4 m_LightViewProjectionMatrix3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These uniforms can be collapsed into a single uniform as an array of 4 mat4s. Then you wouldn't need branching to index into them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did implement this; unfortunately the branching in the shader remained in the end since it also needed to branch for the shadow sourcemap.
|
Pulled this branch and ran the test. Worked flawlessly and looks absolutely amazing! |
Full engine compute shader support would require a setup similar to Material with proper uniforms, defines, versioning, etc. That is a lot of work and Material itself is pretty well baked into the vertex pipeline. Probably best to work with the limited support this PR offers until jme4 rolls around. |
Sample-Distribution Shadow Mapping (SDSM) is a technique for dynamically fitting the shadow maps of a shadow filter to the depth samples that will be needed, resulting in improved shadow quality.
This commit adds the option to use a SdsmDirectionalLightShadowFilter in place of a regular DirectionalLightShadowFilter. Usage requires GL 4.3+ support.
I don't think this is 100% ready for merging immediately, but it does work. I've been told there might be code for compute shaders coming, in which case this can be adapted to use that instead of somewhat kludgily inventing its own wheel. There's a lot around handling specific GL 4.3+ APIs (for example this needs compute shaders) that I'm not sure how engine wants to do it.
Mentioned on the forums here:
https://hub.jmonkeyengine.org/t/reducing-shadow-motion-issues/49289/3
Included is a TestSdsmDirectionalLightShadow sample app. I was using a stripped down version of the Sponza model to test lighting, but it's still pretty big and it didn't seem like large test data gets included in PRs. It's available here, and goes in the jme3-examples directory alongside "town.zip": https://drive.google.com/file/d/18mmwRPe--gskuWjfBfv9nB-dCSzfa1eB/view?usp=sharing
Example with SDSM:

Example without SDSM:

(Note that some parts of the translation from Kotlin to Java I delegated to Claude Code, so if there are still some uselessly verbose parts I didn't catch those can be fixed.)