When one passes the wrong order or number of arguments in a random map script and a vector is involved, the random map script often doesn't
show any kind of error and one can only notice that some entities were not placed or similar.
I cannot imagine any good coming from storing NaN in vectors, since every operation inside (and around?) the prototype assumes numbers.
Details
Details
- Reviewers
vladislavbelov
Is this true for the simulation and GUI usage in general too?
Note that typeof NaN is "number": https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 4650 Build 8100: Vulcan Build (Windows) Build 8099: Vulcan Build Build 8098: arc lint + arc unit
Event Timeline
Comment Actions
Again I ran into this having been useful if committed.
The JS simulation only has a handful of Vectors and doesn't use NaN anywhere.
The C++ simulation converts CFixedVector2D to a JS Vector2D when passing things over to JS.
There are few hundred occurrences of 2D vectors in C++. All of them look like innocent math, but you never know.
Though I would like to get rid of this patch and benefit from the warning, I suppose this should wait for right after the release unless someone can testify that this patch is good.
Comment Actions
The rest of the patch looks good.
| binaries/data/mods/public/globalscripts/vector.js | ||
|---|---|---|
| 31 | I suggest to cover it by quotes to understand that we passed empty string i.e.: "x = '" + uneval(x) + "', y = '" + uneval(y) + "'"; | |
| binaries/data/mods/public/simulation/components/tests/test_Vector.js | ||
| 160 | It'd good to add few tests with string and undefined. | |
Comment Actions
uneval returns quotes around strings.
Notice also that your request assuming that assumption to be true, would also make it impossible to distinguish "1" from "1".
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/uneval
The best argument against this patch is one use case I encountered, determining the min/max of vector components with more than 2 vectors.
We'd start with a vector(infinity, -infinity) and decrease/increase only.
But the vector can start with the values of the first vector or remain undefined to cover this use-case.
Comment Actions
Could add. As mentioned above this is mostly planned for after the release.
What about NaN or Infinity being a legitimate use case?
| binaries/data/mods/public/globalscripts/vector.js | ||
|---|---|---|
| 251 | three | |
Comment Actions
How about introducing a NaNaNVector2D that 'inherits' all Vector2D functions and just replaces the set method?
Comment Actions
The problem is only that using NaN or Infinity in a Vector is rather a nearly singular edge case, so we should rather have typeCheck = true in the vector construction and pass false for the one use case that wants infinity for some reason, right?