bakkemo / elm-collision

Efficient collision detection for Elm
BSD 3-Clause "New" or "Revised" License
10 stars 4 forks source link

Things are kinda brokeny #3

Open Fresheyeball opened 9 years ago

Fresheyeball commented 9 years ago

So, this package wont install with the current standard libs as of the last release. I was prepping a PR to get this to be compatible, but it looks like master is in a weird state. PolySupport has been removed, but its still in comments and docs. Can you tell me a-little bit about the current state of elm-collision?

bakkemo commented 9 years ago

PolySupport was removed after List.maximum was changed to return a Maybe value, forcing error handling concerns into the library (instead of the app where it really needs to be if one is defining their own boundary objects). It was simply there initially because I thought it was probably overwhelmingly the most likely use case, not because it definitely belongs there. That is why I provide the implementation in the README markdown. The frustration with decisions made by the Elm devs, combined with having 0 users contributed to me not bothering to fix the documentation and Elm packaging (as of 15.0 it would work fine if you just installed it manually). I haven't bothered to install anything past Elm 15.0, can you tell me why it doesn't install now? Did some standard libs API change?

On Mon, Aug 17, 2015 at 8:23 AM, Isaac Shapira notifications@github.com wrote:

So, this package wont install with the current standard libs as of the last release. I was prepping a PR to get this to be compatible, but it looks like master is in a weird state. PolySupport has been removed, but its still in comments and docs. Can you tell me a-little bit about the current state of elm-collision?

— Reply to this email directly or view it on GitHub https://github.com/bakkemo/elm-collision/issues/3.

zaidan commented 8 years ago

There is a workaround for the Maybe value of List.maximum: (m, p) = Maybe.withDefault (0, (0,0)) (List.maximum decorated)

The PolySupport example is working fine with Elm 0.16.0. Thank you for this great package!

bakkemo commented 8 years ago

That isn't really a "work around" as it synthetically adds a point (0,0) that most likely is not part of the boundary. You actually make my point about not adding specific boundary representations to the library, since the error handling needs to be on the callee side of things.

Chadtech commented 8 years ago

Is it really adding the point (0,0)? Maybe.withDefault returns (0,(0,0)) only if List.maximum cant return a value. So if we are certain that List.maximum will always have a value to return (say, we know decorate will always have at least one value), then we know (0,0) will never actually be returned by polySupport.

Is this right or wrong? I have also implemented Maybe.withDefault (0,(0,0) into my code, and would like to know if it wont work.

zaidan commented 8 years ago

It really depends on your use case. In my case there are always two non-empty polygons. Both polygons have to be non-empty, otherwise this point is added to the boundary. Using withDefault has no effect as long as your code keeps this invariant. A workaround is always dirty, therefore it is an appropriate term and you should use them with care.

bakkemo is right not adding the PolySupport example to the core library, but adding a fully working example would help other people. Still this library feels kind of broken. The collision function should return "Maybe Bool", because this is the idiom Elm adopted from Haskell. Otherwise this library does not encourage correct error handling and supports empty polygons that does not make sense. Adding this is really simple, we just need a guard for the collision function that checks for empty minks and returns "Nothing". If we want to support clean PolySupport implementations, we could add error handling to the calcMinkSupport function. Then we can add an extra library with a collection of PolySupport functions.

If you can not guarantee non-empty polygons, adding a guard to your function that calls the collision function is safe. But if you think about the sense of empty polygons and emphasize clean design, you will realize that error handling is required for (almost) all use cases.

I have added error handling in pull request #6: https://github.com/zaidan/elm-collision/tree/error-handling

There is also an updated Readme with example polySupport function. If you do not want to handle the error case you can use: Maybe.withDefault False collision ... No point is added to the boundary and defining False for empty polygons is a valid definition, although having empty polygons is a sign of bad input or a bug. Keep in mind, that you will not recognize these empty polygons and make debuging harder.

@bakkemo You wrote that you haven't installed anything past Elm 15.0. Are you still interested in maintaining this package? It would be great if we can publish a release as soon as possible. If you do not have enough time, i can adopt this package and publish it to the Elm repository. Otherwise it would be great if you can give me a feedback.

bakkemo commented 8 years ago

I'll make time tonight (my time) to look at this.

bakkemo commented 8 years ago

I contend that the need for for this particular error handling is an artifact of the data representation. I can think of other boundary objects (or representations) that do not need it. Circles for example can be represented as (x,y,r) : (Float, Float, Float) [ or ((x,y),r) or whatever]. The point is that treating negative radii the same as positive ones by taking the absolute value, you can never pass a value that can fail. this STRONGLY indicates to me that error handling should be outside the library (unless the aforementioned Minkowski object is the only one anyone will ever use.

On the other hand I strongly agree that adding fully worked out examples would help other people (I was actually unaware that there were ANY users of the library). I'm open to any suggestions

zaidan commented 8 years ago

I consider it as an artifact of the interface. If you do not add error handling, you are not able to add error handling to the custom PolySupport function itself. Because it is user-dependent code, there is no guarantee it can't fail. Some code may need to return a "Nothing" other code (like your circle example) does not. Using lists as objects is just one case.

When you give collision the return type "Bool" you guarantee that the result will be either True or False, but the library depends on that custom PolySupport function and can't guarantee it can't fail. Therefore collision can't guarantee it's result is always True or False and "Maybe Bool" is the correct return type, because it allows the result to be "Nothing". Do not think about it like try-catch error handling, it's more like putting the result in a Box that can be empty. If you consider empty as an error or not is up to you. If you do not want to use Maybe, you should not support user-defined callback functions and have to accept you can't support a wide range of use cases.

You wrote it is not valid to add a point to the boundary and you are correct. The only correct result of such a PolySupport function is "Nothing". Using "Maybe Pt" instead of "Pt" as return type does not restrict your circle example and does not make it more complex ("= result" vs "= Just result"), but not allowing the return type to be "Nothing" will restrict a lot of cases. Especially the example from the Readme that seems to be your major use case.

The Elm Community is growing, do not underestimate your package ;-).