Closed codex128 closed 4 months ago
The java code looks ok to me, but I am not experienced enough to do a review of the shader code. So you need approval from a shader expert and a core member for final integration. @riccardobl @stephengold @scenemax3d
Does the PBRBloomFilter work only with PBR materials?
In general, yes, because this filter relies heavily on HDR.
What effect does it have on Lighting materials (the teapot and the box in the test class)?
Same effect, technically, though not noticeable because Lighting.j3md doesn't utilize HDR. I copied TestBloomFilter.java, so that is why the teapot is in the test.
Can you copy/paste a screenshot in the comments please?
This is a multipass bloom filter that uses hardware filtering to increase the samples, similar to the mipmap bloom filter but it doesn't relies on the opengl mipmap generation function (that is driver dependent and not guaranteed to run on the gpu).
This is much better than the default bloom filter and it has a different optimization (uses hardware filtering instead of two pass blur) so it might be pretty much equivalent or even better from a performance standpoint, and it has less dependencies on the driver implementation than the mipmap bloom, so it is better from that side too.
My only suggestion is to find a different name, since calling it PBR might be confusing.
I have one question though, what is the default min filter for the filter getRenderedTexture()
? This should work best with MinFilter.BilinearNoMipMaps , can you check that? @codex128
My only suggestion is to find a different name, since calling it PBR might be confusing.
Good idea, though PBR Bloom is what the original article calls it. Perhaps SoftBloomFilter?
what is the default min filter for the filter
getRenderedTexture()
?
Yup, the default min filter for pass textures is NearestNoMipMaps, so that should be good.
This is a multipass bloom filter that uses hardware sampling, kinda like the mipmap bloom filter-
My only suggestion is to find a different name, since calling it PBR might be confusing.
Good idea, though PBR Bloom is what the original article calls it. Perhaps SoftBloomFilter?
Yes SoftBloomFilter might be a good option.
what is the default min filter for the filter
getRenderedTexture()
?Yup, the default min filter for pass textures is NearestNoMipMaps, so that should be good.
Can you try with BilinearNoMipMaps and show the results? This will enable hardware filtering when downscaling , the result should be smoother. Might be worth trying also Bilinear MagFilter when upscaling.
Can you try with BilinearNoMipMaps and show the results?
Oh, sorry, my bad. :stuck_out_tongue: Here is the result with min=BilinearNoMipMaps and mag=Bilinear:
It's difficult to tell, but I think the default min/mag filters produce slightly rougher glow. For reference:
The differences become more apparent if you zoom in on the glowing sections.
It should be more visible with complex scenes.
One last thing, can you make it on by default but toggleable with setBilinearFiltering(false)
?
I have implemented that. I also added something to suppress the number of downsampling/upsampling passes made so that textures do not have width or height <= 0.
I have implemented that. I also added something to suppress the number of downsampling/upsampling passes made so that textures do not have width or height <= 0.
Just to be sure to not hurt the feeling of any driver, could you please make it <=2 ?
The filter introduced in this PR produces much cleaner and visually natural results than BloomFilter. Only supports the equivalent of GlowMode.Scene (no select object glow).
Basic usage is similar to BloomFilter:
Note: name changed from PBRBloomFilter to SoftBloomFilter.