-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added SoftBloomFilter #2229
Added SoftBloomFilter #2229
Conversation
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
jme3-effects/src/main/java/com/jme3/post/filters/PBRBloomFilter.java
Outdated
Show resolved
Hide resolved
The java code looks ok to me, but I am not experienced enough to do a review of the shader code. |
In general, yes, because this filter relies heavily on HDR.
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.
|
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 |
Good idea, though PBR Bloom is what the original article calls it. Perhaps SoftBloomFilter?
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-
Yes SoftBloomFilter might be a good option.
Can you try with BilinearNoMipMaps and show the results? This will enable hardware filtering when downscaling , the result should be smoother. |
Oh, sorry, my bad. 😛 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. |
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 ? |
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.
Can you rename PBRBloomFinal.frag and PBRBloomFinal.j3md to SoftBloomFinal.frag/j3md?
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.
Perfect. Thank you.
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.