This may avoid copying a few bytes here and there (as in SplitInHalf) but in my opinion it is not worth it. The many curve. and to a lesser extent Curve:: that appear are ugly and cause "churn" see my comment about it here.

However if you have an example model where profiling shows that this change causes a significant speed up or is on the critical path we should consider it.

@iscgar Are you saying that SplitInHalf is doing a deep copy of all the trims? That should not be happening, but if it is we should fix it for performance reasons. BTW we don't care about copying the color or handle, that's very minor.

My biggest with this issue is that names are important. In Solvespace a curve is a one-dimensional curve either in a plane or in 3d space. A surface is a 2-D surface in 3-D space. We don't want to use curve terminology for a surface data structure - that's just confusing.

Second complaint is that this just takes more text to specify a piece of data. Almost every line of the diff is just adding the extra colons and the word "curve" to the code.

This may avoid copying a few bytes here and there (as in SplitInHalf) but in my opinion it is not worth it. The many curve. and to a lesser extent Curve:: that appear are ugly and cause "churn" see my comment about it here.

However if you have an example model where profiling shows that this change causes a significant speed up or is on the critical path we should consider it.

Right now this is indeed only copying a few additional bytes because copying a List<T> is a shallow operation which does not copy the stored items, as List<T> requires an explicit call to .Clear() in order to destroy the items and free the memory.

But as I noted in my reply to your comment there, I'm working on reworking the way List<T> and IdList<T> are implemented in order to speed them up and reduce memory usage. This includes making List<T> an owner of its memory and responsible for tracking and releasing it, and therefore a copy of it performs a deep copy of its underlying storage instead of just copying a pointer.

As part of that change I also made the following conversions to code that copied List<T>:

• Places that were copying a List<T> and clearing the source were changed into performing an std::move()
• Places where items were copied one-by-one using .Add() (in order to make a deep copy) were changed to a simple assignment

In that context, copying the entire SSurface struct can become a heavy operation if there are many edges or trims, and neither of the aforementioned conversions could work in the two cases that are mentioned in the commit message, so I resorted to refactoring the curve data into its own struct in order to allow working on it without running into the potential performance issue.

@iscgar Are you saying that SplitInHalf is doing a deep copy of all the trims? That should not be happening, but if it is we should fix it for performance reasons. BTW we don't care about copying the color or handle, that's very minor.

As I explained above, no deep copy is performed with the existing code.

My biggest with this issue is that names are important. In Solvespace a curve is a one-dimensional curve either in a plane or in 3d space. A surface is a 2-D surface in 3-D space. We don't want to use curve terminology for a surface data structure - that's just confusing.

Second complaint is that this just takes more text to specify a piece of data. Almost every line of the diff is just adding the extra colons and the word "curve" to the code.

Both valid points, and I fully accept your reservations (especially in light of @ruevs 's comment in the other PR about avoiding disturbing the existing code as much as possible). However, assuming my reason for making this change seems reasonable, I'm open to suggestions for a better term, or to thinking of other ways to achieve the same result without introducing another nesting level.

I'm working on reworking the way List<T> and IdList<T> are implemented

You will not be the first one ;-)
I rewrote IdList four years ago
#1002

with help, ideas and inspiration from:
#939 (comment)

@pjanx
#992
#973

@rpavlik
#433
#429

@Evil-Spirit
Evil-Spirit@44eb1a4

Discussions:
#939
#932

In general keep in mind these when working on performance:
performance

Test models to profile with:
#1186

Last time I profiled (about half an year ago, but nothing performance related has changed since) neither List nor IdList were a bottleneck. For example on this #759 (comment) most of the time was spent on axis aligned bounding boxes :-) A torture test model for IdList is here:
#932 (comment)
See here for how bad performance on it used to be: #992 (comment)

and one more:
#841 (comment)

More wisdom from @rpavlik (shallow copy is intentional) #932 (comment)

Test and profile with various models in release mode (with optimisations on and debug info for profiling). A debug build is no indication at all about the real performance and bottlenecks.

In short - I'm very exited that you have started looking deeply into SolveSpace and am very curious what will come out of it.

What platform are you on?

@ruevs thanks for the links to past discussions and ideas! I've seen some of them, but definitely not all of them. I'll make sure to take the information in them into account before I submit my work.

I'm on Linux, but I'm also running on Windows every once in a while.

I added a different approach to achieve the same result, which hopefully addresses the concerns raised (assuming such a change seems reasonable despite not having an impact on the existing implementation).

I intentionally did it by adding a revert commit and then the new approach, so that the two approaches could be compared easily. If the change is acceptable, I'll remove the first two commits.

One possible reason to put the raw Rational Bezier Surface data into a separate struct within SSurface would be to abstract the underlying surface type (later - much much later) so someone could implement other types of surfaces, or just more generalized NURBS surfaces.

So if we allowed that, there must be a mathematical word for "the surface that the trim polygon defines a subset of". We'd want to name it that instead of curve (It must not be curve! ;) and maybe use an abbreviation.

I only used Curve because I didn't want to add to the confusion by having a a type named Surface nested inside SSurface, but if an abbreviation is acceptable, the type can just be RationalBezierSurface and the member abbreviated to rbs.

But that's assuming we indeed want to go this way instead of just changing the two places where this will become relevant with my other changes.

However if you have an example model where profiling shows that this change causes a significant speed up or is on the critical path we should consider it.

Here is a model
SplineIntersectedWith128ConeLikeThings.zip
that (on Windows, 32 bit release build, with LTO, with MSVC 2019) spends 31% of its time in:

SSurface::AllPointsIntersecting
 and then in:
   SSurface::AllPointsIntersectingUntrimmed
      ClosestPointTo(p, &u, &v);
        Vector tryp = PointAt(tryu, tryv);

As you can see currently the bottleneck is not SplitInHalf but in SSurface::ClosestPointTo -> SSurface::PointAt. Unfortunately nothing can be done about SSurface::PointAt (at least that I can think of).

The recursive AllPointsIntersectingUntrimmed that does the SplitInHalf is more interesting. Comenting out the 5 debug lines starting with this one:

Reduces the loading time from 15.1s to 13.9s on my machine. The critical path is still the same, but a bit faster. @phkahler perhaps we should #ifdef DEBUG those lines?

From a performance perspective, I think optimizing low level data structures is a waste of time. Even the OpenMP stuff is kind of a hack (but a really good one). The real performance problems come from algorithms that are O(n^2) and higher. The fix for those things will be a spatial index to reduce the number of things tested against other things. I've got an index in mind but using that would make the code more complex and harder to fix the bugs we still have in the NURBS code. We're still in the "make it work right" phase but some "make it fast" changes are still OK.

BTW a major bottleneck for a lot of things is the silhouette edges being rendered. Turn those of in the GUI ;-)

Even the first item in the old wishlist is related to algorithmic complexity. That is still an O(n^2) algorithm.