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

Adding support for Specular-AA #2141

Merged
merged 15 commits into from Nov 9, 2023
Merged

Conversation

JohnLKkk
Copy link
Contributor

@JohnLKkk JohnLKkk commented Nov 6, 2023

Copy link
Contributor

@codex128 codex128 left a comment

Choose a reason for hiding this comment

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

Just some unused variables/functions that should be trimmed out if possible.

jme3-core/src/main/resources/Common/ShaderLib/PBR.glsllib Outdated Show resolved Hide resolved
jme3-core/src/main/resources/Common/ShaderLib/PBR.glsllib Outdated Show resolved Hide resolved
jme3-core/src/main/resources/Common/ShaderLib/PBR.glsllib Outdated Show resolved Hide resolved
Copy link
Contributor

@codex128 codex128 left a comment

Choose a reason for hiding this comment

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

Ok, that looks good! 👍

@riccardobl
Copy link
Member

riccardobl commented Nov 6, 2023

Should we set this on by default? Are there any disadvantages in doing so? It doesn't seem computationally intensive

@JohnLKkk
Copy link
Contributor Author

JohnLKkk commented Nov 6, 2023

Should we set this on by default? Are there any disadvantages in doing so? It doesn't seem computationally intensive

Personally, I'm inclined to set its default value to True, however, others may not want anti-aliasing to be on by default (for some reason it seems most people use the low quality graphics mode in JME), so I'm not sure if the default here should be changed to True...

@JohnLKkk
Copy link
Contributor Author

JohnLKkk commented Nov 6, 2023

I forgot one thing, I will expose the two parameters Screen Space Variance and Threshold to material parameters tomorrow as well. For descriptions of them, refer to:
https://docs.unity3d.com/Packages/com.unity.render-pipelines.high-definition@15.0/manual/Geometric-Specular-Anti-Aliasing.html

@riccardobl
Copy link
Member

I think we should make an exception in this case, since it won't impact performances too much and it is most likely a wanted feature, i doubt anyone likes aliasing in the reflections, but let's wait for a counterargument .

@zzuegg
Copy link
Member

zzuegg commented Nov 6, 2023

I would say make it default

@oxplay2
Copy link

oxplay2 commented Nov 6, 2023

agree, i think it should be default with just option to disable. (like its option now)

Tho i belive it might be nice to add later to PBRTerrain too? (it do not need to be this pr ofc)

@JohnLKkk
Copy link
Contributor Author

JohnLKkk commented Nov 7, 2023

agree, i think it should be default with just option to disable. (like its option now)

Tho i belive it might be nice to add later to PBRTerrain too? (it do not need to be this pr ofc)

Specular-AA has been added to PBRTerrain.

I think we should make an exception in this case, since it won't impact performances too much and it is most likely a wanted feature, i doubt anyone likes aliasing in the reflections, but let's wait for a counterargument .

I would say make it default

True has been set as the default value, and two material parameters (Screen Space Variance and Threshold) have been added.

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.

Thank you! If there are no further objections, I will merge it within 24 hours.

@riccardobl riccardobl merged commit 3ec59c8 into jMonkeyEngine:master Nov 9, 2023
14 checks passed
@stephengold stephengold added this to the Future Release milestone Nov 9, 2023
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.

None yet

6 participants