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

Adjustments to the buoyancy demo to make the code more easily reusable #263

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sbj42
Copy link

@sbj42 sbj42 commented Oct 6, 2016

I was using the buoyancy demo as a template for my own water simulation code, but I was tripped up on some simple edge cases that were not accounted for in the demo.

This pull request includes "fixes" for those edge cases, so that others might not get caught by them when copying this code. First, the submerged-area calculation hard-coded the water plane position (14728c0). Second, the forces were disproportionately calculated for bodies with different numbers of shapes (fc33f4d).

…hen computing the area of the submerged shape. The plane's position in this demo happens to be 0 so it worked fine, but using the planePosition variable may help avoid a hurdle for people who use this code as a template.
…he number of shapes in the body. This way, two bodies with equal mass and area but with different numbers of shapes will behave the same. Without this change, the forces were disproportionately larger for complex bodies (such as the four-rectangle body2 in the demo), causing them to settle higher in the water than simpler bodies would.
@sbj42 sbj42 mentioned this pull request Oct 6, 2016
@@ -86,7 +86,7 @@
} else if(aabb.lowerBound[1] < planePosition[1]){
// Partially submerged
var width = aabb.upperBound[0] - aabb.lowerBound[0];
var height = 0 - aabb.lowerBound[1];
var height = planePosition[1] - aabb.lowerBound[1];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was caught by this issue as well when I was running my own implementation.

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

Successfully merging this pull request may close these issues.

2 participants