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.

Sign up for GitHub

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

Jump to bottom

Fix bug in BSDF code gen for HW languages #1399

Merged
merged 3 commits into from
Jul 10, 2023
Merged

Fix bug in BSDF code gen for HW languages #1399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb24815
Fix bug with HW codegen of nodes in different contexts
niklasharrysson Jul 6, 2023
7350082
Removed debug code.
niklasharrysson Jul 6, 2023
87976ed
Update GltfPbr implementation graph
niklasharrysson Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions libraries/bxdf/gltf_pbr.mtlx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,6 @@
<!-- Thin-film + Dielectric
Note: Due to limitations in codegen, the base layer BSDF is duplicated (#1035). -->
Copy link
Member

Choose a reason for hiding this comment

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

Can this comment now be removed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Jonathan! I think this comment is meant for the dielectric base layer under thin-film layering. The specular reflection and transmission BSDF's below are also duplicated, in order to have the dielectric layer both with and without thin-film, to blend between. That duplication is needed for another code gen limitation. You can't have the same BSDF node both with and without thin-film (even if it's possible to create such a graph).

However, I have another PR coming up after this one, with the change to thin-film handling that we discussed, where thin-film is set by parameters instead of an extra layer operator. That change will modify this graph as well, and I will remove the comment since it's then no longer valid.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification, @niklasharrysson!


<oren_nayar_diffuse_bsdf name="tf_diffuse_bsdf" type="BSDF">
<input name="color" type="color3" interfacename="base_color" />
<input name="normal" type="vector3" interfacename="normal" />
</oren_nayar_diffuse_bsdf>

<dielectric_bsdf name="tf_transmission_bsdf" type="BSDF">
<input name="weight" type="float" value="1" />
<input name="tint" type="color3" interfacename="base_color" />
Expand All @@ -173,7 +168,7 @@
</generalized_schlick_bsdf>

<mix name="tf_transmission_mix" type="BSDF">
<input name="bg" type="BSDF" nodename="tf_diffuse_bsdf" />
<input name="bg" type="BSDF" nodename="diffuse_bsdf" />
<input name="fg" type="BSDF" nodename="tf_transmission_bsdf" />
<input name="mix" type="float" interfacename="transmission" />
</mix>
Expand Down
3 changes: 3 additions & 0 deletions source/MaterialXGenShader/HwShaderGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,9 @@ void HwShaderGenerator::emitFunctionCall(const ShaderNode& node, GenContext& con
emitLineBegin(stage);
emitOutput(node.getOutput(), true, true, context, stage);
emitLineEnd(stage);

// Register the node as emitted, but omit the function call.
stage.addFunctionCall(node, context, false);
}
}

Expand Down
10 changes: 7 additions & 3 deletions source/MaterialXGenShader/ShaderStage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,14 +365,18 @@ void ShaderStage::addFunctionDefinition(const ShaderNode& node, GenContext& cont
}
}

void ShaderStage::addFunctionCall(const ShaderNode& node, GenContext& context)
void ShaderStage::addFunctionCall(const ShaderNode& node, GenContext& context, bool emitCode)
{
// Register this function as being called in the current scope.
const ClosureContext* cct = context.getClosureContext();
const FunctionCallId id(&node, cct ? cct->getType() : 0);

_scopes.back().functions.insert(id);

node.getImplementation().emitFunctionCall(node, context, *this);
// Emit code for the function call if not omitted.
if (emitCode)
{
node.getImplementation().emitFunctionCall(node, context, *this);
}
}

bool ShaderStage::isEmitted(const ShaderNode& node, GenContext& context) const
Expand Down
5 changes: 4 additions & 1 deletion source/MaterialXGenShader/ShaderStage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,10 @@ class MX_GENSHADER_API ShaderStage
void addFunctionDefinition(const ShaderNode& node, GenContext& context);

/// Add the function call for the given node.
void addFunctionCall(const ShaderNode& node, GenContext& context);
/// This will register the function as being called in the current scope, and code for the
/// function call will be added to the stage. If emitCode is set to false the code for the
/// function call will be omitted.
void addFunctionCall(const ShaderNode& node, GenContext& context, bool emitCode = true);

/// Return true if the function for the given node has been emitted in the current scope.
bool isEmitted(const ShaderNode& node, GenContext& context) const;
Expand Down