Skip to content
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

Merged
merged 18 commits into from
May 15, 2024
Merged

Added SoftBloomFilter #2229

merged 18 commits into from
May 15, 2024

Conversation

codex128
Copy link
Contributor

@codex128 codex128 commented Mar 19, 2024

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:

FilterPostProcessor fpp = new FilterPostProcessor(assetManager);
PBRBloomFilter bloom = new PBRBloomFilter();
fpp.addFilter(bloom);
viewPort.addProcessor(fpp);

Note: name changed from PBRBloomFilter to SoftBloomFilter.

@codex128 codex128 requested a review from capdevon March 19, 2024 19:43
@codex128 codex128 requested a review from capdevon March 20, 2024 11:51
@capdevon
Copy link
Contributor

capdevon commented Mar 20, 2024

  • Does the PBRBloomFilter work only with PBR materials ?
  • What effect does it have on Lighting materials (the teapot and the box in the test class) ?
  • Can you copy/paste a screenshot in the comments please ?

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

@codex128
Copy link
Contributor Author

codex128 commented Mar 20, 2024

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?

@riccardobl
Copy link
Member

riccardobl commented Mar 20, 2024

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

@codex128
Copy link
Contributor Author

codex128 commented Mar 20, 2024

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.

@riccardobl
Copy link
Member

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.

@codex128
Copy link
Contributor Author

Can you try with BilinearNoMipMaps and show the results?

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.

@riccardobl
Copy link
Member

It should be more visible with complex scenes.
One last thing, can you make it on by default but toggleable with setBilinearFiltering(false)?

@codex128
Copy link
Contributor Author

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.

@riccardobl
Copy link
Member

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 ?

@codex128 codex128 changed the title Added PBRBloomFilter Added SoftBloomFilter Mar 23, 2024
Copy link
Member

@riccardobl riccardobl left a 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?

Copy link
Member

@riccardobl riccardobl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Thank you.

@codex128 codex128 requested a review from capdevon March 23, 2024 21:17
@stephengold stephengold added this to the Future Release milestone May 15, 2024
@stephengold stephengold merged commit 76d8a43 into jMonkeyEngine:master May 15, 2024
14 checks passed
@yaRnMcDonuts yaRnMcDonuts modified the milestones: Future Release, v3.8.0 Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants