Skip to content

[lisp/geo/geopack.l] normalize norm vector in optional arugment of vector-angle function. #262

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

Merged
merged 2 commits into from
Jul 11, 2018

Conversation

mmurooka
Copy link
Member

As described in document https://github.com/euslisp/EusLisp/blob/master/lisp/geo/geopack.l#L104, normal argument should be unit length. Default value of optional argument is very bad because (v* v1 v2) does not become unit-length when v1 and v2 have some angle (> 0).
This PR makes euslisp/jskeus#485 test pass.

@mmurooka
Copy link
Member Author

mmurooka commented Feb 18, 2018

If only first commit, efd0371, nan is returned for parallel v1 and v2. So I added second commit, 72cd2b3 7f491ef.
Added nan check of parallel case to test under jskeus, https://github.com/euslisp/jskeus/pull/485/files.

@mmurooka mmurooka force-pushed the vector-angle-normal branch from 72cd2b3 to 7f491ef Compare February 18, 2018 08:28
Copy link
Member

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

@mmurooka
Copy link
Member Author

seems ok except (parallel-thre 1e-10)
how did you find this value?

No reasonable reason, but just intuition. So welcome for any opinion or idea for this.
Variable name of *parallel-threshold* https://github.com/euslisp/EusLisp/blob/master/lisp/geo/geopack.l#L53-L60 matches for the purpose, but 0.01 might be too large.

@YoheiKakiuchi
Copy link
Member

Formar euslisp was developed for 32bit OS.
For 64bit OS, the threshold can be reduce up to 1e-9 times.

see calculation of epsilon:
#260

@mmurooka
Copy link
Member Author

I think that in the case of this PR, low precision of float value becomes more safe result.
For example, if two vectors almost parallel are given,

  • In 32bit OS, if outer product norm is 1e-8 => become zero because of low precision => considered as parallel => return 0[rad] or pi[rad].
  • In 64bit OS, if outer product norm is 1e-8 => larger than parallel threshold (1e-10) => return actual angle (> 0)

@k-okada
Copy link
Member

k-okada commented Feb 20, 2018

@YoheiKakiuchi so 1e-6 for 32 and 1e-15 for 64?

@mmurooka
Copy link
Member Author

mmurooka commented Feb 20, 2018

I find good solution.
What we want to check here is that (normalize-vector (v* v1 v2)) is calculated correctly. So the result of normalized-vector should be checked.
normalize-vector function behaves differently between eus and irteus when zero vector is given, because it is defined in eus and redefined in irteus: https://github.com/euslisp/jskeus/blob/master/irteus/irtmath.l#L610-L620.
So how about 8e2a396 038d5bc ? We don't need to introduce new epsilon in this PR (however I agree epsilon discussion is important).

;; (v* #f(1 0 0) #f(1 0 0)) is #f(0 0 0)
# eusgl
1.eusgl$ (let* ((normal (normalize-vector (v* #f(1 0 0) #f(1 0 0)))))
  (print (list (equal normal (float-vector 0 0 0))
               (equal normal (float-vector *nan* *nan* *nan*))))
  )
(nil t)

# irteusgl
1.irteusgl$ (let* ((normal (normalize-vector (v* #f(1 0 0) #f(1 0 0))))) 
  (print (list (equal normal (float-vector 0 0 0))
               (equal normal (float-vector *nan* *nan* *nan*))))
  )
(t nil)

@mmurooka
Copy link
Member Author

Please review new PR #264 . (I did not break this PR branch so that we can see the discussion with original PR source code later.)

@k-okada
Copy link
Member

k-okada commented Jul 11, 2018

For longer perspective, I think we should change 'norm' behavior and merge #264, but merged for now.

@mmurooka
Copy link
Member Author

For longer perspective, I think we should change 'norm' behavior and merge #264, but merged for now.

Maybe you mean normalized-vector behavior?

@k-okada
Copy link
Member

k-okada commented Jul 11, 2018

@mmurooka you are right,
I think we will change 'normalize-vector behavior in future and merge #264, but merged this for now.

@k-okada k-okada merged commit 1f5a9e9 into euslisp:master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants