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.
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
Remove the Random() function and use a fixed table of arbitrary vecto… #667
Conversation
I like the added determinism (among other things it would help with the testcases), but it's not clear to me why your solution is sufficient. @jwesthues, what do you think? |
Is there an off-by-one? My old Otherwise, I think I like the change. Slightly weird to reuse the random numbers in successive coordinates, but I can't articulate any specific harm. If we're going to have a fixed table, then maybe it would be better to rationally design that than just use random numbers? I'd have to think about what's best though. Close to the coordinate axes, but not exactly so you don't hit on edge? You could experiment on problem models. While you're at it, I believe there's some |
Probably. I thought it was a 6 iteration loop, so needed 8 entries. Is this actually wrong?
I thought random looking numbers would be good to avoid any odd coincidences. Picking points on a regular polyhedron gives a good distribution of directions, but what if for some reason that causes issues with some regular polyhedral model? Doesn't seem likely but I thought something more randomish may be a good idea? The original code creates vectors all in the first octant (signs all positive) and potentially very short if the random numbers happen to be small. I placed the negative values so that successive vectors have signs +++ ++- +-+ -++ so they are in 4 of the 8 octants. The segments use a point plus and minus these vectors so I figured that covers all 8 octants. Small, medium, and large values rotating through the coordinates keeps them pointing in different directions even without the negative values. It wasn't completely random :-) Also scaled them up to lengths roughly on the order of 10 because they're rarely added to anything small. Overlapping the vectors was just to avoid a lot more numbers and cache waste. I'm open to other suggestions.
Yes, the triangles are randomized. I'm not really into that yet. The NURBS parallelism is currently done at the surface level, but the triangle meshes are just one big shell aren't they? That will require a different approach to parallelism. OTOH I'd rather spend time fixing the NURBS issues. It still bothers me that this fixed my red line issue - it seems like there should have been enough variation in the random vectors to avoid whatever the problem was. |
I think the loop iterates seven times from 0 to 6 (since on the second-last iteration Common directions (axis-aligned, 30/45/60 degree rotations from that, etc.) are definitely bad, since that will cause the ray to hit on edge and the test to fail--you want the ray to hit another surface as far away as possible from that surface's trim curves. Your design procedure seems plausible to me, perhaps worth a comment explaining. |
…rs in raycast.cpp This also fixes issue solvespace#666.
OK loop fixed and comment added to the table. |
This eliminates the custom Random() function which was only used in raycast.cpp. That used rand() which is usually not thread-safe or causes cache slowdowns when used in multiple threads. A simple table of arbitrary vectors is used instead.
This fixes issue #666 but I'm not entirely sure why. Possibly better edge classification with the longer vectors in this set or the better distribution of directions? BTW that issue is independent of OpenMP use.