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

Don't try to ReParameter symbols not in the group #1693

Merged
merged 1 commit into from
Jun 9, 2023
Merged

Don't try to ReParameter symbols not in the group #1693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 6 additions & 4 deletions src/liboslexec/instance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,17 +109,19 @@ ShaderInstance::findsymbol(ustring name) const


int
ShaderInstance::findparam(ustring name) const
ShaderInstance::findparam(ustring name, bool search_master) const
{
if (m_instsymbols.size())
for (int i = m_firstparam, e = m_lastparam; i < e; ++i)
if (m_instsymbols[i].name() == name)
return i;

// Not found? Try the master.
for (int i = m_firstparam, e = m_lastparam; i < e; ++i)
if (master()->symbol(i)->name() == name)
return i;
if (search_master) {
for (int i = m_firstparam, e = m_lastparam; i < e; ++i)
if (master()->symbol(i)->name() == name)
return i;
}

return -1;
}
Expand Down
2 changes: 1 addition & 1 deletion src/liboslexec/oslexec_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1158,7 +1158,7 @@ class ShaderInstance {

/// Find the named parameter, return its index in the symbol array, or
/// -1 if not found.
int findparam(ustring name) const;
int findparam(ustring name, bool search_master = true) const;

/// Return a pointer to the symbol (specified by integer index),
/// or NULL (if index was -1, as returned by 'findsymbol').
Expand Down
9 changes: 8 additions & 1 deletion src/liboslexec/shadingsys.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3213,7 +3213,14 @@ ShadingSystemImpl::ReParameter(ShaderGroup& group, string_view layername_,
return false; // could not find the named layer

// Find the named parameter within the layer
int paramindex = layer->findparam(ustring(paramname));
int paramindex = layer->findparam(ustring(paramname),
false /* don't go to master */);
Comment on lines +3216 to +3217
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a suggestion:

First, look for it with search_master=false. If that succeeds, we're done.

If that search fails, try again with search_master=true. If that succeeds, then you know it's the case that the parameter did exist, but was optimized away.

I think that in such a case, ReParameter should return true. It's not a failure in the same sense as using a parameter that doesn't exist in the shader, or using a parameter not marked as interactive. It's simply that changing the value of the parameter has no effect on the appearance, so you don't need to communicate the new value to the GPU or whatever. But I still consider that a success in terms of changing the value and being able to get the right picture when you rerender.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remember that I'm simultaneously working on another fix in which if you try to ReParameter something that isn't allowed to vary (say, because not folding it would make something illegal for the GPU), the false it returns can be interpreted by the renderer as "if I want to vary that parameter, I need to re-send the shader group and re-JIT". So I don't want to throw back false failures for harmless things where we can draw the right picture without re-compiling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, done.

if (paramindex < 0) {
paramindex = layer->findparam(ustring(paramname), true);
if (paramindex >= 0)
// This param exists, but it got optimized away, no failure
return true;
}
if (paramindex < 0)
return false; // could not find the named parameter

Expand Down