Fixed baking artifacts on concave faces when enabling AA jittering
ClosedPublic

Authored by Lukas Stockner (lukasstockner97) on Apr 21 2015, 7:41 PM.
Tags
None
Tokens
"Love" token, awarded by redonwhite."Love" token, awarded by MeshLogic."Love" token, awarded by bliblubli."Love" token, awarded by monio.

Details

Summary

This patch fixes (at least for me) the artifact problem mentioned in T43388.
The problem was that while both u and v coordinates are correctly clamped to be <= 1, for valid barycentric coordinates u+v must also be <= 1.
This patch just clamps v to be below 0 and 1-u, which will always give valid values (since u is already clamped to 0 to 1).

Diff Detail

Repository
rB Blender
Lukas Stockner (lukasstockner97) retitled this revision from to Fixed baking artifacts on concave faces when enabling AA jittering.Apr 21 2015, 7:41 PM

The 'USE_BAKE_JITTER' was causing some unintended side-effects.
Did you test the patch with the sample files posted along T40369 and T43388?

The side-effect was because of these out-of-triangle locations. For concave parts of the mesh, this causes the ray origin to be inside the object, therefore producing a black spot. For convex parts, the origin is outside of the mesh, so no problem there.
I tested the monkey.blend from T43388 and the artifacts are gone, the pasteall link in T40369 is offline.

it's not totally correct fix as far as i remember looking into the issue. The actual issue is that BVH ray intersection in the blender side gives incorrect results (barycentric coordinates outside of 0..1 range) presumably due to some precision issues. Think i've mentioned this in the related commit messages or in one of the linked reports. IMO that's the first thing to be addressed first -- make sure correct coordinates are passed to baker.

Would it be enough to do the clamping to correct barycentric range after the jittering, no matter whether it's enabled or not? This way, wrong coordinates both from jittering and from the input would be corrected, and if a precision issue is the source of the wrong results, clamping shouldn't visibly change the result (apart from removing artifacts)...

It looks good so far as the results go. The monkey head is coming out error free. The texture is 1024x1024 with 1024 samples.
Both model and sky are white(1.0, 1.0, 1.0)

I'm not comfortable with this being used for SVM at the moment. There does not seem to be any advantage for SVM and there may yet be situations where this does cause problems.

For SVM there will be a performance penalty and with no positive points to counter-balance the extra code.

@Lukas Stockner (lukasstockner97), you shouldn't masquerade issues coming from bad ray intersection by clamping, you should rather fix the root of the issue and make sure correct coordinates are passed to the kernel.

As for clamping after jitter -- it's more compliated, because you can't really just clamp. that'd cause some correlation artifacts.

I don't understand the discussion here really. Fundamentally the issue is that the bake API should be doing antialiased baking, that is rasterizing the triangle multiple times with small offsets and doing multiple AA samples per pixel.

This code in the baking kernel is kind of a workaround for that, it tries to take multiple AA samples in the same pixel, by offsetting the barycentric coordinates a bit. That's not the same as you don't get antialiasing of geometry, but it helps gives some better results until we improve the bake API.

Now if you offset the barycentric coordinates, you might end up outside the triangle, so the barycentric coordinates need to be clamped in such cases. There is no BVH ray intersection involved here, it's not about precision as far as I know.

If this fixes the artifacts that originally caused this code to be disabled, then to me committing this seems fine as an intermediate step until we improve the bake API.

This revision is now accepted and ready to land.Feb 18 2016, 10:04 PM

@Brecht Van Lommel (brecht), the issue is that even without the jitter enabled in the kernel it wasn't so rare case when original coordinate passed to the kernel was already outside of a triangle. Maybe this got solved over all the fixes since my original investigation, maybe not. If it was not addressed, even with this patch applied you'll still have artifacts and would need to go fixing original intersection code om the bake_api side.

Ok, I haven't come across files where the bake API produces barycentric coordinates outside the triangle. The barycentric coordinate differentials for selected to active were broken, but I committed a fix for that today.

If there's cases where we have barycentric coordinates outside the triangle then I'll fix them. The bake API has some weak points, but ensuring that the u and v it produces satisfy u >= 0, v >= 0 and u+v <= 1 should not be difficult.

Miki (MeshLogic) added a comment.EditedFeb 19 2016, 7:09 PM

I think the biggest advantage of this AA patch is when you are using procedurally generated cycles materials, viz my procedural wood material https://developer.blender.org/T43388. With official Blender, the baked texture looks looks really awful and useless. Using this patch, the baked texture looks very nice and clean like the output from render. So, I hope this patch will be included in Blender soon, because I'm tired myself to compile Blender each time an official version is released to be able to use procedural cycles materials for baking. Even if this patch was turned off by default and was possible to turn it on by some command would be great. The picture bellow shows the difference of baking quality by official Blender and using this patch (both examples baked into texture 1024x1024).

PS: When I was doing those tests with cycles materials, increasing render samples in official version (as many were advising me) didn't help at all. Only this patch has helped to get decent baking results.

I've been trying a few different files, and looking into the code, I can't find any way the bake API can generate barycentric coordinates outside of the triangle. We can add some asserts in the code to ensure the input coordinates are valid before offsetting them, so it doesn't go unnoticed when there are bugs here.

I don't see any reason not to commit this now?

A compromise would be to add a checkbox "Use jittered samples" or similar instead of statically enabling/disabling it - that way, default behaviour wouldn't change but people could still use it (kind of like an experimental feature).

Generally, though, I don't really understand the reason against committing this patch.
Even if there are remaining issues in the Bake API, the argument is essentially "fixing this bug is useless because there is another bug as well", which seems weird to me since the clamping in the current Cycles baking code is definitely wrong in combination with jittering.

@Brecht Van Lommel (brecht), there were fixes related to ray-tri and ray-bvh intersection in blender after i was doing initial investigation. The issue might simply be gone with those, i never had time to re-run tests after all those changes. Think we should totally add an assert in the bake_api side. Possible stopper for the patch is bcon level, but i'm fine considering it just a bugfix if it's all well-tested.

@Lukas Stockner (lukasstockner97), no, don't add more options. The reason is -- such changes masquerades actual bugs, which makes investigation much more tricky. This we should avoid doing as much as possible. If there's no known issues in the bake_api, add assert in there and go ahead with the patch.

This revision was automatically updated to reflect the committed changes.