ands / lightmapper

A C/C++ single-file library for drop-in lightmap baking. Just use your existing OpenGL renderer to bounce light!
1.44k stars 134 forks source link

Assert fail on boundary check #11

Closed huxingyi closed 6 years ago

huxingyi commented 6 years ago

Hi, very cool and useful project, I am integrating it into my 3d modeling software.

I found maybe a bug when I am testing. it crashes on this assert sometimes,

https://github.com/ands/lightmapper/blob/85fafede9d666c74397400024dc8b62013c457db/lightmapper.h#L419

Assertion failed: (x >= 0 && x < ctx->lightmap.width && y >= 0 && y < ctx->lightmap.height), function lm_getLightmapPixel, file src/qtlightmapper.h, line 472.
Printing description of ctx->lightmap:
(lm_context::(anonymous struct)) lightmap = {
  width = 512
  height = 512
  channels = 4
  data = 0x000000010de6b000
}
...
x = 512
...
Printing description of ctx->meshPosition:
(lm_context::(anonymous struct)) meshPosition = {
  ...
  rasterizer = (minx = 510, miny = 482, maxx = 512, maxy = 488, x = 512, y = 482)
  ...
}

I guess the max bound of xy should be the width/height minus one? https://github.com/ands/lightmapper/blob/85fafede9d666c74397400024dc8b62013c457db/lightmapper.h#L1059-L1060

I changed it to,

ctx->meshPosition.rasterizer.maxx = lm_mini((int)bbMax.x + 1, ctx->lightmap.width - 1);
ctx->meshPosition.rasterizer.maxy = lm_mini((int)bbMax.y + 1, ctx->lightmap.height - 1);

and it worked quite well, but I am not sure if this is a right fix, or did i do something wrong? I have ported it to the Qt environment to make use of the QOffscreenSurface, by wrapping all the functions to a class and changed the shader code to make it work on 120 version, but leave all the other codes untouched.

ands commented 6 years ago

Hey, thanks :) I've seen your project and it is pretty cool too! I'm pretty sure your fix is correct. I will check and integrate it today after work. Thanks for reporting it! :)

Regarding the porting work: At some point I want to make it easier to swap out the parts that depend on the graphics API, so that it would be easier to port the project to other APIs or environments and not end up with a totally custom version of the lib that is hard to update. :)

PS: I've seen the screenshot you posted of the ambient occlusion feature. If you actually already supply vertex normals for smooth surfaces, you may want to try increasing the last parameter in lmCreate to e.g. ~5.0f or so. It may help to get rid of the flat shaded look (if that is what you want). :)

Have fun! ands

huxingyi commented 6 years ago

Thanks :D Separating the graphics API would make integrating much easier, actually, I am a little worried about the upgrade when I was integrating to Qt environment. I am looking forward to the future version of lightmapper. Thanks for your suggestion, I will try to play the lightmapper options, right now, I just finished the porting work. And one more question, I see the generated shadow in the texture map looks like expanded a little bit from the boundary border, although the final rendered model looks good, is this right?

ands commented 6 years ago

Hey :) Could you clarify what you mean with your question? Can you show me the problem/effect in a screenshot maybe?

If you are talking about the lightmap values outside the UV mapped regions, that's what the dilation image post-processing passes do (lmImageDilate calls). They help to fill in missing light samples (due to invalid camera positions while rendering, e.g. when camera frustums intersect with geometry and "sees" the insides) with values from valid neighbor texels. It also helps on lower mip map levels of the texture if you use mip mapping. Since mip map levels are downsampled versions of the original texture, texels contain the average of some higher resolution mip level texels. So, if you're sampling a lower resolution mip level at the edge of a piece from your UV map, with dilation you would still get somewhat valid values, instead of an average with possibly completely missing data. This page tries to describe the problem: http://wiki.polycount.com/wiki/Edge_padding

huxingyi commented 6 years ago

Thanks for your clear and detailed answer :D now I understood and learned, there is no issues with the expanded shadow at all.

ands commented 6 years ago

Okay, cool :) I submitted the fix (see attached commit). Actually, the rasterizer bounding conditions were somewhat inconsistent throughout the code. I had to fix it in some more places. Thanks a lot for the report! :)