Changes for #458 and #465 #466
Conversation
…move limit on number of sections. Delete definition of Revolved struct.
phkahler
changed the title
|
(I can't merge this PR as-is because the last commit does two completely unrelated things, so it's going to wait until I have time to manually untangle it.) |
|
@whitequark Regarding the 3rd commit: The Revolved struct was fixed length, so by using std::vector there is no need to limit number of sections, and that was the last use of the struct so its definition can be deleted entirely. I would have done the last 2 commits as one but I wanted to get the lathe one in first in case I messed up the bigger one (which turned out to be small too). If anything, the first commit is completely unrelated to the other two. But when I committed those to my repo, github wanted to automatically lump all three together. Wasn't sure how to get the second two as a separate PR so I just edited it. To me this might have been 2 PRs, but IMHO there is no problem with the individual commits. |
Ah, sorry, I misunderstood. Indeed there is no problem. |
| @@ -740,21 +733,21 @@ void SShell::MakeFromHelicalRevolutionOf(SBezierLoopSet *sbls, Vector pt, Vector | |||
|
|
|||
| // If this input curve generated a surface, then trim that | |||
| // surface with the rotated version of the input curve. | |||
| if(revs.d[0].v) { // not d[j] because crash on j==sections | |||
| if(revs[0].v) { // not d[j] because crash on j==sections | |||
There was a problem hiding this comment.
fwiw, most uses of .v are no longer necessary #448 - except I think for this one (treating it as a bool) which I couldn't achieve with a non-invasive change.
|
@rpavlik Can we make an explicit comparison to zero here? These are handles to surfaces, and it's just using zero as an indicator that there isn't one here. If integer comparison isn't allowed, could you define NO_SURFACE which is really zero but of the right type? @whitequark It's true that these odd little quirks in the code - like the .d and .v stuff - are a big part of the learning curve to get into SolveSpace development. But it's not just that, for me it's C++ itself. In this function, we're creating surfaces (objects), doing things with them, getting handles, passing those around, and then just letting everything go out of scope. I was constantly wondering where things are being allocated and what was keeping them around. There are really only two ways to get through that. One is to dig in to all the definitions, functions, constructors, destructors, and such. The other is to decide to just trust that it was written correctly and use everything the way it already is. I chose the later approach ;-) The details become clearer over time. |
|
Yes, you could do the NO_SURFACE - a similar NO_ENTITY Is used elsewhere. |
No description provided.