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

Add LineTest #1567

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

Add LineTest #1567

wants to merge 10 commits into from

Conversation

wei-shiuan-chang
Copy link
Contributor

This test is to increase the test coverage, and the feature we tested was the distance method in the Line class.
We used a dot product method, and a cross product method to find the distance, which had the same results. However, we found out an issue that the jMonkey’s output for distance was different, and should be incorrect according to how the actual math works. The test case was intended to give a failed test because the calculations for the distance seemed to not match the actual calculations.

@pspeed42
Copy link
Contributor

The thing is that JME's math in Line looks right. I can't make sense of your findNearestPoint(), though. It seems to be treating direction like an end point instead of a direction vector.

For example: float vecX = line.getDirection().x - line.getOrigin().x;

...makes no sense to me.

Math should be:
vec relative = point - origin
float project = relative dot direction
result = origin + direction * project

I wonder if you add an assert to test your own methods against each other if they will even match.

@wei-shiuan-chang
Copy link
Contributor Author

wei-shiuan-chang commented May 27, 2021

I am not sure if we misunderstood the definition of distance in JME. According to the javadoc of JME, the direction is the second point other than the origin, so the vecX you mentioned is the vector with origin as the tail and the specific point as the head. The findNearestPoint() is to find out the point on the line that has shortest distance with the specific point.
There are some reasons why we thought there is an issue in the class:

  1. Test case:
    We tested it with the following line and point:
    Vector3f origin = new Vector3f(0,0,0);
    Vector3f direction = new Vector3f(2,0,0);
    Vector3f point = new Vector3f(2,0,0);
    However, the distance we got from the Line.distance was 6.0, differed from 0 that we got from our methods.

  2. As the javadoc says a line is defined as "infinite" along two points, we thought the distance should be the same if the irreducible fraction of the direction is the same. However, there are some cases, such as changing the direction from (1,0,0) to (2,0,0), that will cause different distances.

@pspeed42
Copy link
Contributor

Vector3f direction = new Vector3f(2,0,0);
...is not a direction vector. It's a location.

Normalize it.

@wei-shiuan-chang
Copy link
Contributor Author

I understand that the direction is not a vector, so vecX = line.getDirection().x - line.getOrigin().x is to make it as an x component vector.
I did some tests and I think a line:
lineParameter /= (direction.xdirection.x+direction.ydirection.y+direction.z+direction.z);
should be added to normalize the variable in the distanceSquared() method in JME.

@pspeed42
Copy link
Contributor

JME expects direction to already be normalized. This is true across the entire codebase. Everywhere you see "direction" then it should be normalized. Some places have asserts to check it for you. Some places normalizeLocal() for you (possibly redundantly) but probably MOST places expect direction to already be normalized.

So in your test, change:

        Vector3f origin = new Vector3f(0,70,0);
        Vector3f direction = new Vector3f(1,8,0);
        Vector3f point = new Vector3f(32,1,8);

To:

        Vector3f origin = new Vector3f(0,70,0);
        Vector3f direction = new Vector3f(1,8,0).normalizeLocal();
        Vector3f point = new Vector3f(32,1,8);

...if you want accurate results.

@wei-shiuan-chang
Copy link
Contributor Author

Thanks a lot for the information. We will look into our test methods and make it works to increase the test coverage.

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