Avoid tweaking tangent points of WrapCylinder when single object wrap…

[aymanhab]

…ping, allow for optimization if disableVisualization is true. Fix test cases accordingly

Fixes issue #<issue_number>

Brief summary of changes

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

• no need to update because...
• updated.

---

This change is 

[nickbianco]

@aymanhab, based on the the changes here and the CI logs, I think it's fine to create a new standard for testMocoInverse.

Reviewed 2 of 19 files at r2, 26 of 26 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aymanhab)

---

OpenSim/Common/ModelDisplayHints.h, line 110 at r7 (raw file):

    /** Turn off visualization completely, only use API/modeling.
    Meshes will not be loaded, pathwrappig intermediate points not computed.
    Intentionally there's no reverse API to turn on visualization downstream.

Could you add a note that indicates that excluding the intermediate path point calculations doesn't affect things like moment arm calculations? Also, typo pathwrappig --> pathwrapping.

---

OpenSim/Moco/MocoProblemRep.cpp, line 185 at r7 (raw file):

    // Grab a writable state from the copied model -- we'll use this to disable
    // its constraints below.
    m_model_disabled_constraints.updDisplayHints().disableVisualization();

You probably want to make this call on m_model_base as well. Or if this setting persists upon model copy, then you could replace this call with a call on m_model_base.

---

OpenSim/Simulation/Wrap/WrapCylinder.cpp, line 546 at r7 (raw file):

    vv.normalize();

    SimTK::UnitVec3 plane_nomral(uu % vv);

Typo here that might be breaking some results: plane_nomral --> plane_normal. Also, this would redefine the plane_normal instantiation above.

---

OpenSim/Simulation/Wrap/WrapCylinder.cpp, line 667 at r7 (raw file):

    // WrapTorus creates a WrapCyl with no connected model, avoid this hack
    if (!_model.empty() && !getModel().getDisplayHints().isVisualizationEnabled() &&
            aWrapResult.singleWrap) {

Why is this only updated when visualization is disabled?

[aymanhab]

Reviewable status: 33 of 36 files reviewed, 16 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

---

Applications/Forward/test/testForward.cpp, line 176 at r9 (raw file):

Previously, carmichaelong wrote…

same as above too

See comments above.

---

OpenSim/Simulation/Wrap/WrapCylinder.cpp, line 667 at r7 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Ah, I see. Would it not be better to wrap the code below in an else statement and get rid of the return call?

Done.

---

OpenSim/Simulation/Wrap/WrapCylinder.cpp, line 706 at r13 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Here's another! Same comment as below.

There's a reason nobody over the last 10+ years wanted to touch this to eliminate this goto line. I have no plans to refactor the code as it is error prone and has lots of heuristics with no test cases. This PR is exclusively to improve performance in common cases, so I'd avoid general code cleanup if there're potential side-effects as is the case here.

---

OpenSim/Simulation/Wrap/WrapSphere.cpp, line 253 at r13 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Oh wow, an actual goto statement. My gut reaction out-of-context is to try to avoid this, but not sure how messy things could get. I know there's some valid goto use cases but my feeling is this could be avoided.

See comment above regarding code cleanup/refactoring and why I'm avoiding it in this PR

---

OpenSim/Tools/Test/testControllers.cpp, line 415 at r9 (raw file):

Previously, carmichaelong wrote…

should the standard be updated instead of tolerance changed?

See comments above

[aymanhab]

Reviewable status: 33 of 36 files reviewed, 16 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

---

OpenSim/Simulation/Wrap/WrapEllipsoid.cpp, line 889 at r11 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Replace with Simbody operation?

Mtx::Normalize is different from Vec3.normalize in that it returns zero vector if norm is small, also not necessarily done in place. I'm making these changes gradually but initial full text replacement everywhere broke some test cases so I had to back off. I'd suggest we do not hold this PR for other cleanup.

[aymanhab]

Reviewable status: 27 of 37 files reviewed, 16 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

---

OpenSim/Simulation/Wrap/WrapCylinder.cpp, line 489 at r11 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Any reason these three usages of Mtx::Normalize are not replaced with normalize()?

Moved Mtx::Normalize to WrapMath::Normalize which is a bit different from normalize() in that it returns the magnitude and that it zeros the vector norm if small. All other methods of Mtx were inlined/substituted with simbody methods

[aymanhab]

Reviewable status: 27 of 37 files reviewed, 16 unresolved discussions (waiting on @carmichaelong and @nickbianco)

---

OpenSim/Simulation/Wrap/WrapEllipsoid.cpp, line 538 at r11 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Replace with Simbody operation?

See comments above

---

OpenSim/Simulation/Wrap/WrapSphere.cpp, line 242 at r11 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Replace with Simbody operation? Same goes for the rest of the calls in this file.

Comments above.

[nickbianco]

@aymanhab, left a few more comments. Totally agree about not changing the goto stuff, per our dev meeting discussion. However, I do have some thoughts related to potentially removing or at least renaming Normalize. Let me know what you think.

Reviewed 1 of 6 files at r13, 9 of 9 files at r14, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @aymanhab and @carmichaelong)

---

OpenSim/Simulation/Wrap/WrapEllipsoid.cpp, line 571 at r14 (raw file):

        WrapMath::Normalize(r2w2, r2w2);

        if ((~r1p1*r1w1) > 0.0 || (~r2p2*r2w2) > 0.0)

Ah, these "greater-than-zero" conditionals are probably why Normalize has the if-statement. My preference would be to remove the if-statement in the function (actually, replace the function with Simbody's normalize()) and replace these conditionals with (~r1p1*r1w1) > SimTK::Eps. Normalize is used in many places, so this change could change results when vector magnitudes are close to zero, but this change should be very very small since the comparison is against SimTK::Epsanyway.

---

OpenSim/Simulation/Wrap/WrapMath.h, line 79 at r14 (raw file):

     * Normalize a vector.
     *
     * If aV has a magnitude of zero, all elements of rV are set to 0.0.

Do you have a sense if the if-statement in this function is necessary? For example, is the output ever checked to be equality with zero? I don't like that the zeroing out here is hidden within the function. If it needs to stay, then I would advocate for renaming for clarity (i.e., NormalizeOrZero, etc).

[aymanhab]

Reviewable status: 31 of 37 files reviewed, 10 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

---

OpenSim/Simulation/Wrap/WrapMath.h, line 79 at r14 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Do you have a sense if the if-statement in this function is necessary? For example, is the output ever checked to be equality with zero? I don't like that the zeroing out here is hidden within the function. If it needs to stay, then I would advocate for renaming for clarity (i.e., NormalizeOrZero, etc).

It's impossible to guess in general because it depends on what arguments are passed in, and that depends on Model configurations. I totally agree with renaming and just pushed a commit for that.

[aymanhab]

Reviewable status: 31 of 37 files reviewed, 10 unresolved discussions (waiting on @aymanhab, @carmichaelong, and @nickbianco)

---

OpenSim/Simulation/Wrap/WrapMath.h, line 79 at r14 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

It's impossible to guess in general because it depends on what arguments are passed in, and that depends on Model configurations. I totally agree with renaming and just pushed a commit for that.

Throwing an exception at the else clause to see if it's reached, causes 4 of our test cases (which very thinly cover wrapping) to fail, so it appears this is not a rarity and suggest it's safer to leave as is.

[carmichaelong]

Thanks @aymanhab, overall looking good. I noticed some places in which normalize() wasn't reverted back to WrapMath::NormalizeOrZero() (btw this is a much clearer name) though some of these cases are OK when the logic beforehand ensures a vector that isn't too small.

That being said, before checking/noting all those cases, I think @nickbianco's comment in WrapEllipsoid.cpp L571 should be addressed first, just in case that this was the source of the test errors and could simplify the code to using normalize() more.

Reviewed 1 of 3 files at r10, 3 of 9 files at r14, all commit messages.
Reviewable status: 31 of 37 files reviewed, 2 unresolved discussions (waiting on @aymanhab and @nickbianco)

[aymanhab]

Thanks @carmichaelong I responded to @nickbianco comment below.

Reviewable status: 31 of 37 files reviewed, 2 unresolved discussions (waiting on @nickbianco)

---

OpenSim/Simulation/Wrap/WrapEllipsoid.cpp, line 571 at r14 (raw file):

Previously, nickbianco (Nicholas Bianco) wrote…

Ah, these "greater-than-zero" conditionals are probably why Normalize has the if-statement. My preference would be to remove the if-statement in the function (actually, replace the function with Simbody's normalize()) and replace these conditionals with (~r1p1*r1w1) > SimTK::Eps. Normalize is used in many places, so this change could change results when vector magnitudes are close to zero, but this change should be very very small since the comparison is against SimTK::Epsanyway.

@nickbianco Typically in this code sign is used to decide "sidedness" (e.g. if dot product is +ve is one scenario, -ve is a different scenario) as such adding slop of SimTK::Eps would skew the test incorrectly to one side. That's why I'd try to avoid modification to the semantics at all. If you prefer I can undo all the normalize introductions and restore NormalizeOrZero so it's clear we're only changing name and no functionality whatsoever. Please let me know what you think.

[nickbianco]

Thanks @aymanhab. I also agree NormalizeOrZero is much clearer. However, I've made one final argument for removing it, since we have a chance to remove all remnants of the Mtx code here. I think it's at least worth trying removing the "zero" branch of that function to see how our existing tests change.

It might be easier to discuss some of this at dev meeting tomorrow.

Reviewed 6 of 6 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aymanhab)

---

OpenSim/Simulation/Wrap/WrapEllipsoid.cpp, line 571 at r14 (raw file):
I'd have to spend some time to better understand what's happening here, but wouldn't the "zero" part of NormalizeOrZero also being skewing results?

If you prefer I can undo all the normalize introductions and restore NormalizeOrZero so it's clear we're only changing name and no functionality whatsoever.

I guess we should be consistent with whatever we do. If we're concerned that removing the NormalizeOrZero function changes stuff, then we shouldn't replace it with normalize() anywhere. But my intuition is that the "zero" part of this function isn't needed everywhere, so I don't like that it's baked into this function. If we have a chance to remove all the weird stuff from Mtx here it's worth a shot.

---

OpenSim/Simulation/Wrap/WrapMath.h, line 79 at r14 (raw file):

Previously, aymanhab (Ayman Habib) wrote…

Throwing an exception at the else clause to see if it's reached, causes 4 of our test cases (which very thinly cover wrapping) to fail, so it appears this is not a rarity and suggest it's safer to leave as is.

It might be reaching the "else" statement in situations where it's not really necessary to zero out the vector. It would be better to try removing the "else" branch and see if it changes any tests results. We could also specifically check if the if-statement in WrapEllipsoid changes too.

---

OpenSim/Simulation/Wrap/WrapMath.cpp, line 87 at r15 (raw file):

    double denom = cross_prod.normSqr();

    if (EQUAL_WITHIN_ERROR(denom,0.0)) {

Here's another case where it might be better to check against SimTK::Eps.

---

OpenSim/Simulation/Wrap/WrapSphere.cpp, line 319 at r15 (raw file):

    r1bm.normalize();
    r2am.normalize();
    r2bm.normalize();

Per my comment above: if we're not using normalize() everywhere, then we shouldn't use it here.

[aymanhab]

Good suggestion @nickbianco and it actually doesn't break tests, however we know our coverage of wrapping is thin and I can artificially cause coincident points to be passed in to the function causing division by zero, so I'll leave the method name as NormalizeOrZero, restore it everywhere in wrapping code for consistency as you suggested and we should be done with this PR without introducing any unexpected side effects due to other unrelated cleanup. We can revisit later in another PR but Mtx is now gone and WrapMath is localized and reduced.

Reviewable status: 33 of 37 files reviewed, 4 unresolved discussions (waiting on @aymanhab and @nickbianco)

[nickbianco]

Sounds good @aymanhab. LGTM.

Reviewed 3 of 4 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @aymanhab)

[nickbianco]

Reviewable status: 36 of 37 files reviewed, 1 unresolved discussion (waiting on @aymanhab and @nickbianco)

---

OpenSim/Actuators/ModelProcessor.h, line 129 at r19 (raw file):

            Model modelFromFile(path);
            model = std::move(modelFromFile);
            model.updDisplayHints().disableVisualization();

Actually, I just noticed this. I don't think we need this here, since we already display visualization within Moco, and users creating a Model with ModelProcessor might want visualization enabled.

[nickbianco]

@aymanhab, I just noticed something, left one last quick comment.

Reviewable status: 36 of 37 files reviewed, 1 unresolved discussion (waiting on @aymanhab and @nickbianco)

[aymanhab]

@aymanhab, I just noticed something, left one last quick comment.

Reviewable status: 36 of 37 files reviewed, 1 unresolved discussion (waiting on @aymanhab and @nickbianco)

Sure, undone.

[nickbianco]

Reviewed 1 of 1 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aymanhab)

[aymanhab]

Thanks @nickbianco 👍 much appreciated.