Skip to content

Conversation

@louhy
Copy link
Contributor

@louhy louhy commented Jun 13, 2019

Proposed fix for issue #1121. Since it's visual not sure if there's any good options for a test, but we'll have at least one which uses this shape when I update the pending physics GImpact shape test.

@stephengold
Copy link
Member

For testing, how about using it in a Geometry, reading the bounding volume, and comparing against the expected bounds?

@louhy
Copy link
Contributor Author

louhy commented Jun 16, 2019

Went a bit overkill on this one... world bounds tests added for pretty much all shapes (seemed weird to just do it for PQTorus). Test fails without fix, succeeds with fix.

@stephengold
Copy link
Member

Very nice. Thank you.

@stephengold
Copy link
Member

How about stricter tests of the bounding volumes? Either verify that all the vertex positions are within the bounding volume, or check the bounds against hand-calculated minima, or both.

@louhy
Copy link
Contributor Author

louhy commented Jun 16, 2019

See how that one looks, I did make a few assumptions but I believe they're safe. Should be a more useful test like this I think. Put it together pretty quick, so double check that it makes sense. Reversing the vertex component order to 2,1,0 made it fail as I expected but I didn't check much else.

@stephengold
Copy link
Member

Looks good to me. Unless there's some objection in the next day or so, I'll integrate this PR.

@stephengold stephengold merged commit 1f3245b into jMonkeyEngine:master Jun 18, 2019
@stephengold stephengold added this to the v3.2.4 milestone Jul 7, 2019
stephengold pushed a commit that referenced this pull request Jul 10, 2019
* #1121-updateBound() call for PQTorus geometry updates

* World bounds test added for #1121

* World bounds test enhancements for #1121

* Updated test comment
@louhy louhy deleted the pqtorus-bounds branch July 11, 2019 00:57
@stephengold stephengold added the bug Something that is supposed to work, but doesn't. More severe than a "defect". label Sep 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something that is supposed to work, but doesn't. More severe than a "defect".

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants