sampotter / fluxpy

Fast thermal modeling and radiosity in Python.
11 stars 5 forks source link

wrong illumination geometry in debug-compressed-form-factor-matrix #40

Closed steo85it closed 2 years ago

steo85it commented 2 years ago

I think debug-compressed-form-factor-matrix branch has an issue similar to #4 . When running haworth.py from that branch, I get the left result in the figure below (E, above; T, below) - on the other hand, when I run the same experiment from a9f3dc13 , I get the result on the right. The latter makes more sense, considering the shape of the crater and shadowed areas therein.

immagine immagine

I checked and the issue which solved #4 still looks "solved": now that I know that the problem doesn't come from my own code/meshes/changes, I will try to track it down checking differences among the commits. Ah, this happens with both embree or CGAL: it has to be related to some of the clean-ups, but not to this "fundamental change". Also, I went back in history in the master branch only to avoid having to debug unrelated errors in the haworth.py test, not because the issue showed up (I'll check now at which point it appears in master, if any).

@sampotter

nschorgh commented 2 years ago

For a (x,y,z) mesh there is no fundamental way to decide what is inside or outside, so the choice whether N[:,2]>0, N[:,2]<0, or N=radial counts as outside has to be made manually.

steo85it commented 2 years ago

Yes, but in the given example the mesh was the same (and derived from stereo projected values...) and we still get very inconsistent results. I'm fine with providing the correct N, but I'd like the code to treat it consistently.

steo85it commented 2 years ago

I verified that the latest revision of the master branch still gives consistent results with the "expected behaviour". Things actually change between 8f8c05cc and cb3dcbbb .

sampotter commented 2 years ago

Well, this definitely looks like a bug in how to decide the direction of the surface normal (should be directed outward), or possibly the sun direction vector (should be directed from the triangle to the sun).

This looks like a bug in haworth.py, i.e. user error in the script, not a bug in the library itself...

steo85it commented 2 years ago

Unless I lost track of things, I've been running the same identical version of haworth.py with those 2 commits, and I only get the correct result with one of them (the older one, 8f8c05c, clearly ^^). Still, I tried to play with shape.py, where most changes happened between those 2 commits, but couldn't pin-point a specific change for the moment.

sampotter commented 2 years ago

Understood. I'll take a close look at these commits and figure out what happened.

sampotter commented 2 years ago

Okay, I think I fixed this. It was a stupid mistake. I forgot to negate the result of is_occluded in TrimeshShapeModel.get_direct_irradiance. See https://github.com/sampotter/python-flux/commit/2e4d8c8781aa22c0cc714f39aca8ee9e9afe676d. Here's what I get from Haworth:

haworth_E haworth_T haworth_T_shadow

steo85it commented 2 years ago

Yay, this looks great! Thanks for finalizing the search and solving it. It looks solved on my side, too.