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

Optimizer bug fixes: instance merging, output assignment elision #378

Merged
merged 2 commits into from Jun 17, 2014
Merged

Optimizer bug fixes: instance merging, output assignment elision #378

merged 2 commits into from Jun 17, 2014

Conversation

lgritz
Copy link
Collaborator

@lgritz lgritz commented Jun 14, 2014

Two bug fixes:

  1. We try to merge multiple identical copies of instances within a shading network by elaborately checking if all their parameters and connections are different. But we overlooked one factor: if the corresponding parameters in the two instances were delcared with DIFFERENT lockgeom overrides. This could cause confusion for sure, because under such circumstances, the parameters don't mean the same thing in the two instances (one will search for userdata/primvars, the other will not).
  2. Elision of output parameter assignments failed to take into account whether the output parameter was a designated renderer output (in which case it obviously MUST perform the assignment, or the value won't be where the renderer needs it to be).

@lgritz lgritz changed the title Bug fix in instance merging -- differing lockgeom should invalidate merge Optimizer bug fixes: instance merging, output assignment elision Jun 15, 2014
@lgritz
Copy link
Collaborator Author

lgritz commented Jun 15, 2014

Amended the patch to include a second bug fix (which was the apparent true cause of the bug I was chasing all along -- the instance merging fix is a freebie).

@lgritz
Bug fix in instance merging -- differing lockgeom should invalidate m…
19818b5 
…erge.

We try to merge multiple identical copies of instances within a shading
network by elaborately checking if all their parameters and connections
are different. But we overlooked one factor: if the corresponding
parameters in the two instances were delcared with DIFFERENT lockgeom
overrides.  This could cause confusion for sure, because under such
circumstances, the parameters don't mean the same thing in the two
instances (one will search for userdata/primvars, the other will not).
@lgritz
Optimizer bug fix: outparam_assign_elision was overly aggressive when
25eb2b7 
the output param in question was a renderer output (and thus HAD to
perform the assignment to put the value in the right place).
@fpsunflower
Copy link
Contributor

LGTM

@lgritz lgritz merged commit 25eb2b7 into AcademySoftwareFoundation:master Jun 17, 2014
@lgritz lgritz deleted the lg-lockgeom branch June 17, 2014 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@lgritz @fpsunflower