| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Going through all of my NURBS regression tests, this fails with a file named wrong2.slvs I'll try more variations. Going over every curve seemed to be the problem. Still appreciate feedback, this is looking kind of ulgy IMHO but still somewhat contained. |
Sorry, something went wrong.
|
I need to debug this... not enough stack in my brain to think about it "on paper" (the phone screen) :-) |
Sorry, something went wrong.
|
@ruevs If you look at a simplified test using triangle mesh: I suspect going over all the curves was killing cache performance in addition to that being a complex test (nearest point to curve). Every curve was being checked against every curve and then every surface in the other shell. Maybe I'll try this surface bounding box test and then check the ends of that surface trim curves instead of the surface corners. |
Sorry, something went wrong.
|
Updated with MakeMaxMin, changed the comment, pulled the OMP out into a different PR. |
Sorry, something went wrong.
There was a problem hiding this comment.
OK now I understand thoroughly what you are doing. And after you visual explanation above I think you are doing what you intend to with this code. And now I understand why it helps.
So apart from the few comments in this review I think it is fine to merge this.
Another important point I want to make. Jonathan's logic that, if this is needed, then surface curve intersection is not doing it's job still stands. I tried to dig a little, but did not get far. However I did look at this:
solvespace/src/srf/surface.cpp
Line 203 in 0761339
And I think somewhere we may have a bug in an equivalent (hypothetical) "CurveEntirelyOutsideBbox" logic. But I did not find it and am out of time for now.
Sorry, something went wrong.
|
@ruevs I changed it back to using curve end verticies. It's faster and fixes wrong2.slvs as well. Turns out the performance problem was solved by using bounding boxes prior to doing math like ClosestPointTo(). Should it stay two commits or should I squash them? |
Sorry, something went wrong.
…ile still fixing the NURBS issues resolved by that commit with only modest speed penalty. The performance is significantly improved by using bounding box tests on curves prior to doing complex intersection testing.
|
@phkahler I apologize for the impertinence but I pushed a small cosmetic improvement to the code. NFC. The performance is great.
|
Sorry, something went wrong.
No problem at all. BTW how do you push changes to a PR like that? Going to merge now ;-) |
Sorry, something went wrong.
|
I think that if you check the box "allow maintainers to change" or something when creating the pull request then it gives rights to them to push to your branch. So I have a remote for you: So: |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Two commits. One fixes the huge performance regression and is only a little slower than prior to the regression. 18.5 seconds compared to 193 seconds. Prior to the NURBS fix that same test took a bit over 15 seconds. That seems reasonable for adding more code in order to fix an issue. The second commit here adds OpenMP parallelism to the same part of the code and bring the test down to 11.x seconds.