Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can this scratchVector be removed? #217

Closed
pixelzoom opened this issue Oct 4, 2023 · 6 comments
Closed

Can this scratchVector be removed? #217

pixelzoom opened this issue Oct 4, 2023 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

In NumericalEngine.ts, this looks like an old code-review comment that was not addressed. According to git hiistory, it was added by @samreid for code review issue #88. I'm going to promote it to a TODO.

        // REVIEW: Can this scratchVector be removed?
        body1.forceProperty.value = body1.forceProperty.value.plus( scratchVector.set( force ) );
        body2.forceProperty.value = body2.forceProperty.value.minus( scratchVector.set( force ) );
pixelzoom added a commit that referenced this issue Oct 4, 2023
@pixelzoom
Copy link
Contributor Author

I'm guessing that the question is whether the use of scratchVector in these 2 lines can be removed. Not whether scratchVector can be deleted entirely. (@samreid if you recall, please clarify.)

@AgustinVallejo can't we just add and subtract force directly? Like this:

-        body1.forceProperty.value = body1.forceProperty.value.plus( scratchVector.set( force ) );
+        body1.forceProperty.value = body1.forceProperty.value.plus( force );
-        body2.forceProperty.value = body2.forceProperty.value.minus( scratchVector.set( force ) );
+        body2.forceProperty.value = body2.forceProperty.value.minus( force );

@AgustinVallejo
Copy link
Contributor

I'm not entirely sure, since then I've forgotten the exact reason why we added scratchVector there. @jonathanolson can you chime in?

@pixelzoom
Copy link
Contributor Author

Looking at the other uses of scratchVector, they look appropriate. scratchVector is used to avoid modifying the values of in the positions, velocities, and accelerations arrays.

pixelzoom added a commit that referenced this issue Oct 4, 2023
@pixelzoom
Copy link
Contributor Author

In the above commit, I removed the 2 uses of scratchVector, since I'm certain that they are unnecessary.

But to confirm that the result of the computations is the same, I fuzz tested with this bit of code (which was not commited):

        const saveForce = force.copy();

        // Verify that the computation for body1 is the the same with and without scratchVector.
        const body1Force1 = body1.forceProperty.value.plus( scratchVector.set( force ) );
        const body1Force2 = body1.forceProperty.value.plus( force );
        assert && assert( body1Force1.equals( body1Force2 ) );

        // Verify that the computation for body2 is the the same with and without scratchVector.
        const body2Force1 = body2.forceProperty.value.minus( scratchVector.set( force ) );
        const body2Force2 = body2.forceProperty.value.minus( force );
        assert && assert( body2Force1.equals( body2Force2 ) );

        // Verify that force was unchanged as the result of not using scratchVector above.
        assert && assert( saveForce.equals( force ) );

@pixelzoom
Copy link
Contributor Author

@AgustinVallejo please review and close if OK.

@AgustinVallejo
Copy link
Contributor

Looks reasonable, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants