XboxDev / nxdk-pdclib

The Public Domain C Library (adapted for original Xbox / nxdk toolchain)
http://pdclib.rootdirectory.de
Creative Commons Zero v1.0 Universal
19 stars 9 forks source link

Implement hypot.c #57

Closed AJenbo closed 9 months ago

AJenbo commented 1 year ago

This function is required for the DevilutionX port of Diablo, see https://github.com/diasurgical/devilutionX/issues/6746

JayFoxRox commented 1 year ago

Yes, looks good. Initially I was a bit skeptical about reusing other math.h functions in the implementation, but we already do it for others (like exp in sinh etc.)


However, the commit name is missing the "xbox: " prefix. I'll merge this as soon as the commit is renamed (if nobody else does it)

@XboxDev I'm not sure if GitHub allows renaming in the web-ui with "Squash and merge" for single commits, but we could try enabling that option to avoid holding back PRs like this (due to minor style issues).

GXTX commented 1 year ago

Yes, looks good.

Deviates from the standard.

Page 273: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3088.pdf

The hypot functions compute the square root of the sum of the squares of x and y , without undue
overflow or underflow. A range error occurs for some finite arguments.

Which is why it's a separate function from just sqrt.

AJenbo commented 1 year ago

GitHub does allows renaming in the web-ui with "Squash and merge" for single commits, which would be a lot easier then for me to do it 😅

JayFoxRox commented 1 year ago

Which is why it's a separate function from just sqrt.

Yes. We should create an issue after merge (about the precision). I can tell it should be higher precision (like Fused Multiply-Add) and can avoid precision errors.

Here, on x86/x87, the intermediate might be calculated in higher precision (80 bits), so it probably avoids a bunch of issues (except for hypotl which actually demands that precision).

Eitherway, the proposed implementation is still good enough (to me): it's better than just assert'ing.

AJenbo commented 1 year ago

However, the commit name is missing the "xbox: " prefix. I'll merge this as soon as the commit is renamed (if nobody else does it)

Fixed

AJenbo commented 9 months ago

Is there anything missing before this can progress?

We are also lacking an implementation for strtod() for the upcoming release version since it is needed by Lua which are are adding as a way to mod the game.

thrimbor commented 9 months ago

Just a lack of time on my side, sorry. As for stdtod(), you'll have to provide your own implementation (or take one from a third party that you can just drop in) temporarily.