Have you seen @Evil-Spirit's Eigen port in #225? I was planning to merge that today, actually...

Regardless of anything else, you really shouldn't drop the entire source of Eigen right into this repository. Use https://github.com/eigenteam/eigen-git-mirror as a submodule.

oops, no, I hadn't... Yeah, my bad on the dropping the source in. Not sure why I did that last year. When I went through and fixed it, it was just easier to drop a new one instead of adding the submodule.

Rebased this (along with fixing the extlib thing, now using a submodule). Looked at the stuff by evil-spirit, looks cool, but don't have the understanding of the solver innards to safely rebase it without breaking things.

@rpavlik could you have a look at PR #721 ? I know he did a more extensive update and had lengthy discussion with JW about it, but adding new dependencies without breaking CI is currently way outside my territory.

So I shall look into this eventually, but I definitely wouldn't block 3.0 on it.

@rpavlik No problem, 3.0 is imminent. Soon after that I'll probably try to merge the evil spirit solver improvements that don't involve Eigen and then there will be plenty of time for the rest. BTW I wanted to email you but can't find contact info. Feel free to ping me - same ID at g mail.

OK, updated this.

I honestly don't know enough about the solver to give any helpful feedback.
All I know is that I also run into the limitations of the solver quite quickly (even on a M1 Pro) and for that reason think it is a great idea to bring in Eigen and it's benefits.

I am hoping this can be combined with Evil-Spirit's changes, but am afraid this will make the merge of those changes harder? (I didn't try or look at it extensively though)

I'll probably give a stab at combining the 2 Eigen PR's soon.

I'll probably give a stab at combining the 2 Eigen PR's soon.

@vespakoen don't.

@rpavlik did already! #386 (comment)

Better review his.

OK, I have updated this to contain the un-merged commits. These are all pretty simple replacements, I think most of these profiled as a "problem" back in the day when I did them.

OK, so using the benchmark tool (nice! I don't remember that being there previously) and a "normal" model for me, this reduces the time to load from 0.185s to 0.165s. We did def. pick up a regression, probably in that openmp jacobian, since previous master here gave me 0.161, but I'm bisecting now.

ok, now I can't repro the 0.185. Trying a release openmp build now...

Yeah I should just not do performance testing on a laptop on battery I guess.

Rebased on top of #1183 to avoid that regression.

Anyway, I think these are at least performance neutral compared to #1183 as far as I can tell, possibly a performance improvement. They were a performance improvement back in pre-3.0, but even if they're just a readability improvement: Eigen allows pretty clear, concise expressions which is lovely.

@rpavlik a while back, @ruevs made changes to our vector class adding C++ 11 style return { ... } to the methods. That was a solid win in performance. I'm not in favor of adding Eigen functions in place of ours unless there is a clear advantage. OTOH the faster this gets, the more significant each improvement gets! So I guess I'm saying to retest for performance wins when adding Eigen functions.

I don't have data for these anymore, but they made a visible interactive improvement while editing as well as a profiling improvement back when I wrote these in 2016. Since then I've changed jobs, had another kid, done many other things, and rebased this countless times, so I no longer have that data. But I clearly remember both the min altitude and tangents at being big bottlenecks at that time. I imagine I was using tangent arcs (fillets) which were not used in my test model yesterday because the whole thing was round and because I basically learned they don't work well in stock Solvespace. If you don't want to merge these that's fine, but please close this pr then so I stop spending time rebasing it and trying to submit it for review and integration, I'll just carry it on a local tree.

I'll close it then. I believe there is more improvement in ratpoly to be had without introducing a new API.