Curious behaviour of `quat`, given three arguments

[simon-brooke]

The signature of quat, for three arguments, is [^Float x ^Float y ^Float z]. This would lead the innocent programmer to believe that numbers are expected, but in fact they're not.

jme-clj.core=> (quat 1.0 2.0 3.0)
Execution error (ClassCastException) at jme-clj.core/quat (core.clj:225).
java.lang.Double cannot be cast to com.jme3.math.Quaternion

jme-clj.core=> (quat (quat 1 2 3 4) (quat 4 5 6 7) 2)
#object[com.jme3.math.Quaternion 0x43700dc8 "(7.0, 8.0, 9.0, 10.0)"]

This is because the arity three constructor for Quaternion expects two quaternions and a float.

I suspect what you intended could be achieved by:

(defn quat
  "Returns a [Quaternion](https://javadoc.jmonkeyengine.org/v3.4.0-stable/com/jme3/math/Quaternion.html) 
   object. If no args are passed, the object will
   have the values <0, 0, 0, 1>; if three arguments `x`, `y`, `z` are passed, such
   that all are numbers which can be cast to floats, the object will have the values
   <`x`, `y`, `z`, 1.0>;  if four arguments `x`, `y`, `z`, `w` are 
   passed, such that all are numbers which can be cast to floats, the object 
   will have those values.

   An exception will be thrown if any argument is passed which is not a number, or
   is too large to be cast to a float.
   "
  ([]
   (Quaternion.))
  ([^Float x ^Float y ^Float z]
   (Quaternion. x y z 1.0))
  ([^Float x ^Float y ^Float z ^Float w]
   (Quaternion. x y z w)))

[ertugrulcetin]

@simon-brooke thanks for reaching out! It seems that I did not test it well enough, I'm happy to accept PR for that one 🙌🏻

[simon-brooke]

OK, I'm working on a lot of documentation. I'll drop you at least one pull request later this weekend. Would you prefer

1. one pull request with just code fixes and a separate one with documentation, or
2. just one pull request?

[ertugrulcetin]

One pull request should be fine I think 👍🏻

[simon-brooke]

OK, I'm running slower than I'd like, the weekend's over. But I'm working on it. You can get a view of progress on my fork, which is here.

[ertugrulcetin]

Wow, looks so cool. It seems that you put so much effort!

[ertugrulcetin]

@simon-brooke it seems that linting is broken on master after the PR, could you check and fix it, please?

[simon-brooke]

What are you seeing/which linter are you using? If the problem is

At src/jme_clj/core.clj:null:
Consider using:
  and
instead of:
  #(and %1 %2)

(reported by Kibit), the following patch fixes it. It's also better, more idiomatic code. Shall I submit a new pull request?

illuminator:jme-clj simon$ git diff
diff --git a/src/jme_clj/core.clj b/src/jme_clj/core.clj
index 9307b96..156eec5 100644
--- a/src/jme_clj/core.clj
+++ b/src/jme_clj/core.clj
@@ -307,7 +307,7 @@
    (Quaternion.))
   ([x y z]
    (cond (and (instance? Quaternion x) (instance? Quaternion y) (number? z)) (Quaternion. x y z)
-         (reduce #(and %1 %2) (map number? [x y z])) (Quaternion. x y z 1.0)))
+         (every? number? [x y z]) (Quaternion. x y z 1.0)))
   ([^Float x ^Float y ^Float z ^Float w]
    (Quaternion. x y z w)))
 

[ertugrulcetin]

Yes please

[ertugrulcetin]

did you run lein lint?

[simon-brooke]

My apologies, I appear to have missed this. I have now run lint, and added a new commit to the pull request; there are eleven remaining instances of 'long lines' failures, but each of these is due to a link which cannot be broken or shortened.