tommyettinger / jbump

Java port for bump.lua
Apache License 2.0
31 stars 6 forks source link

Normals are Integers #5

Closed raeleus closed 4 years ago

raeleus commented 4 years ago

According to the spec, normals should be reported only as the values -1, 0, and 1. This PR changes the normals' type from float to integer. This is to encourage users to use "==" comparisons and reduce confusion. This does not change the typical ways users will implement a normal (ie. checking direction or multiplying a vector).

I've made a number of small changes across the lib, but the test still works. One can argue that it is an unnecessary change. I found it as a point of confusion when I was learning the lib. I made a lot of unnecessary "< 0", "> 0", and "isZero()" comparisons because I wasn't sure that there were no float errors.

tommyettinger commented 4 years ago

This looks like a worthwhile change. I should point out that IntPoint doesn't implement .equals(), which could be bad, but it's an easy thing for me to add along with a hashCode().

raeleus commented 4 years ago

Thanks for doing that, but it also means those other point classes need to be updated too 😁

tommyettinger commented 4 years ago

Already did for Point, I think. That means I can use a HashMap<Point, Cell> in place of the quite-bad HashMap<Float, HashMap<Float, Cell>> used in World. Most sane development wouldn't use a hash of a float at all... If there's some agreed-upon epsilon value, I can make Point compare and hash using that epsilon. 1.0f/1024.0f is a nice value, it can be expressed exactly with 0x1p-10f, and it's easy to use bitwise ops with that when it's converted to an int.