Fix missing output of shader parameters when returning from a block #263

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@brechtvl
Contributor

brechtvl commented Jun 18, 2013

If you have these two shaders:

shader test_a(output float f = 0.0)
{
    if(P[0] >= 0.0) {
        f = 1.0;
        return;
    }
}
shader test_b(float f = 2.0)
{
    printf("%f\n", f);
}

and connect them like this:

./testshade --layer layer_a test_a --layer layer_b test_b --connect layer_a f layer_b f

Then it will output 0.0. When removing the return; statement from the first shader, which you would expect to have no influence, the output will be 1.0.

The return; statement would skip transferring the layer outputs into downstream shader's inputs, so I moved that code into the exit_instance_block. The tests pass with this fix, though I feel like I'm missing something obvious as I would expect someone to have encountered this bug already.

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 18, 2013

Member

Not only does this seem like a good fix, but would you believe that at this VERY MOMENT (and unfortunately for the past couple days), I've been tracking down a bug related to early return that mysteriously led to uninitialized values. Though it seems like an astonishing coincidence, I think it's very possible that they are the same underlying bug and you found the fix. (Ugh, you had a nice short repro case, I had been unable to narrow it down and only had spurious failures in a shader network containing 703 nodes!)

Let me try your patch out on our shader and see what happens.

Member

lgritz commented Jun 18, 2013

Not only does this seem like a good fix, but would you believe that at this VERY MOMENT (and unfortunately for the past couple days), I've been tracking down a bug related to early return that mysteriously led to uninitialized values. Though it seems like an astonishing coincidence, I think it's very possible that they are the same underlying bug and you found the fix. (Ugh, you had a nice short repro case, I had been unable to narrow it down and only had spurious failures in a shader network containing 703 nodes!)

Let me try your patch out on our shader and see what happens.

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 18, 2013

Member

Your solution is crashing on my example, I think there must be some "edge case" you haven't considered in your small example, but I think you're 90% of the way there. I'm tracking down the problem...

Member

lgritz commented Jun 18, 2013

Your solution is crashing on my example, I think there must be some "edge case" you haven't considered in your small example, but I think you're 90% of the way there. I'm tracking down the problem...

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 19, 2013

Member

Whoops, looks like I was wrong, my crash was due to my own code, screwed something up in the immense amount of debug scaffolding I had erected.

Your solution looks 100% correct to me!

I'll run some more tests and then merge. Might take until tomorrow, I need to take off for the day.

Thanks!

Member

lgritz commented Jun 19, 2013

Whoops, looks like I was wrong, my crash was due to my own code, screwed something up in the immense amount of debug scaffolding I had erected.

Your solution looks 100% correct to me!

I'll run some more tests and then merge. Might take until tomorrow, I need to take off for the day.

Thanks!

@brechtvl

This comment has been minimized.

Show comment Hide comment
@brechtvl

brechtvl Jun 19, 2013

Contributor

Ha, that's an interesting coincidence, this nice simple code was reported to the Blender bug tracker by Michel Anders. Glad to hear it helped solve your bug.

Contributor

brechtvl commented Jun 19, 2013

Ha, that's an interesting coincidence, this nice simple code was reported to the Blender bug tracker by Michel Anders. Glad to hear it helped solve your bug.

@fpsunflower

This comment has been minimized.

Show comment Hide comment
@fpsunflower

fpsunflower Jun 19, 2013

Member

LGTM

Nice catch!

Member

fpsunflower commented Jun 19, 2013

LGTM

Nice catch!

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 19, 2013

Member

LGTM, will merge in a few minutes, as soon as my testsuite is finished.

Great work, Brecht! While I would like to think that I was on the verge of finding the same solution, it's also possible that I could've sunk multiple additional days into tracking it down, as the only time I'd caught the bug in action was in the middle of one of the biggest shader networks I'd ever seen, and I still didn't quite understand why it was ending up with uninitialized values (though there was circumstantial evidence that it had something to do with an early 'return').

Member

lgritz commented Jun 19, 2013

LGTM, will merge in a few minutes, as soon as my testsuite is finished.

Great work, Brecht! While I would like to think that I was on the verge of finding the same solution, it's also possible that I could've sunk multiple additional days into tracking it down, as the only time I'd caught the bug in action was in the middle of one of the biggest shader networks I'd ever seen, and I still didn't quite understand why it was ending up with uninitialized values (though there was circumstantial evidence that it had something to do with an early 'return').

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 19, 2013

Member

Merged.

Member

lgritz commented Jun 19, 2013

Merged.

@lgritz lgritz closed this Jun 19, 2013

@lgritz

This comment has been minimized.

Show comment Hide comment
@lgritz

lgritz Jun 19, 2013

Member

Brecht, are there particular Blender/Cycles mail lists or forums I should be monitoring to keep abreast of any OSL related issues popping up?

Also, as much I'm thankful and thrilled that you can fix things like this and send patches, I know you have lots of other things on your plate, so please know that especially for things deep in the guts of the OSL system like this, you should always feel free to post a test case to the mail list or as an issue on GH, and I'm happy to try to track it down.

Member

lgritz commented Jun 19, 2013

Brecht, are there particular Blender/Cycles mail lists or forums I should be monitoring to keep abreast of any OSL related issues popping up?

Also, as much I'm thankful and thrilled that you can fix things like this and send patches, I know you have lots of other things on your plate, so please know that especially for things deep in the guts of the OSL system like this, you should always feel free to post a test case to the mail list or as an issue on GH, and I'm happy to try to track it down.

@brechtvl

This comment has been minimized.

Show comment Hide comment
@brechtvl

brechtvl Jun 19, 2013

Contributor

Bugs get reported to our bug tracker but I don't think you need to monitor that. Unless there's another such unlikely coincidence, though in this case it was reported just yesterday too.

What I've been doing is check if the bug is specific to our OSL integration and if not submit an issue, or if I think I can fix it quickly give it a try myself.

The only OSL bug from our tracker that has not been fixed is issue #233, but it has a simple workaround.

Contributor

brechtvl commented Jun 19, 2013

Bugs get reported to our bug tracker but I don't think you need to monitor that. Unless there's another such unlikely coincidence, though in this case it was reported just yesterday too.

What I've been doing is check if the bug is specific to our OSL integration and if not submit an issue, or if I think I can fix it quickly give it a try myself.

The only OSL bug from our tracker that has not been fixed is issue #233, but it has a simple workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment