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

Code simplication idea: auto-create Specular* BxDFs when Microfacet* with 0 roughness are requested #30

Closed
bhouston opened this issue Sep 24, 2015 · 2 comments

Comments

@bhouston
Copy link

These two code patterns (here as pseudo code) are used a lot in the creation of the materials:

var isSpecular = ( uRoughness === 0 && vRoughness === 0 );
if( isSpecular )
    bsdfs.add( new SpecularReflection( reflectance, fresnel ) );
else
     bsdfs.add( new MicrofacetReflection( reflectance, microfacetDistribution, fresnel ) )

And

var isSpecular = ( uRoughness === 0 && vRoughness === 0 );
if( isSpecular )
    bsdfs.add( new SpecularTransmission( transmittance, 1, ior, mode ) );
else
     bsdfs.add( new MicrofacetTransmission( transmittance, microfacetDistribution, 1, ior ) )

I view these two patterns as optimization at the wrong level of abstraction. I would suggest instead that inside MicrofacetTransmission it knows how to use the logic of the optimized SpecularTransissiion BxDF when roughness is 0, or there is a factory function that will return a SpecularTranmission BxDF when the roughess parameters are 0.

Then you can implement this optimization pattern once in a central location and then it is applied everywhere, rather than just in the materials where one remembers to do this roughness dependent switching between these two BxDFs.

If one did this change and the one suggested in #29, the glass material would be simplified from:

var isSpecular = ( uRoughness === 0 && vRoughness === 0 );

var microfacetDistribution = null;
if( ! isSpecular )
    microfacetDistribution = new TrowbridgeReitzDistribution( uRoughness, vRoughness );

if( reflectance !== BLACK )
    var fresnel = new FresnelDielectric( 1, ior );
    if( isSpecular )
        bsdfs.add( new SpecularReflection( reflectance, fresnel ) );
    else
        bsdfs.add( new MicrofacetReflection( reflectance, microfacetDistribution, fresnel ) )

if( transmittance !== BLACK )
    if( isSpecular )
        bsdfs.add( new SpecularTransmission( transmittance, 1, ior, mode ) );
    else
        bsdfs.add( new MicrofacetTransmission( transmittance, microfacetDistribution, 1, ior ) )

To just the very clear and concise:

var microfacetDistribution = new TrowbridgeReitzDistribution( uRoughness, vRoughness );
var fresnel = new FresnelDielectric( 1, ior );
bsdfs.add( new MicrofacetReflection( reflectance, microfacetDistribution, fresnel ) );
bsdfs.add( new MicrofacetTransmission( transmittance, microfacetDistribution, 1, ior ) );

Which to me would be a major improvement for readability, clarify and for maintenance.

@bhouston
Copy link
Author

Probably not important. Closing.

@wjakob
Copy link
Collaborator

wjakob commented Sep 25, 2015

Hi Ben,

those are definitely good ideas, but it's too late at this point to integrate them into PBRTv3. We will keep it in mind for the fourth edition.

-Wenzel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants