-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add inverse hyperbolic functions asinh(), acosh() & atanh() #7110
Comments
I tested the pull request (godotengine/godot#78404), it works. I also had a look at the code, and found it to be a very straightforward implementation. Now, when
Be aware that on Perhaps you have an argument for infinity that I have not thought of? Godot has |
This is pretty much equivalent to doing |
Sorry, I just saw this comment before the PR comment- I wasn't sure about what exactly to do (as I mention on the proposal). I thought of NaN, but like @AThousandShips perfectly says I ended up using infinity/-infinity as to mimic the solution on the other functions "out of range", in which it was decided to "not break" calculations (send NaN). I also saw that gdscript has no issue working with INF (as it is called there) values -but I am not like 100% sold on that, I am open to other suggestions if there are other alternatives, as long as those are documented. INF will not be issue-free, just will behave "better" than NaN. |
I think
What's worth explicitly noting though (in the docs) is that for |
Yes, I will try to make this clear on the docs with an amend, as it is a less straightforward decision than on the other cases. |
From what @kleonc says it seems i should have asked if clamping as desired, not if INFINITY was desired (yes, I'm aware of the limits, I mentioned them). Notice I wasn't asking about What @lawnjelly says here godotengine/godot#78404 (comment) makes sense to me (NAN is worse than -INF and INF), although we can check for all of them with Either way I agree the documentation must be clear on what it does. |
See godotengine/godot#78404#issuecomment-1600729386 for the built documentation of acosh and atanh of the latest push. Given it is a reference manual, I wanted to be precise but not extend too much about why, as it should be fast to read. |
Thank you for the review and merge! Excited to be able to use this for my catenaries on Godot! 💖 |
GDScript has the following built-in trigonometry functions: - `sin()` - `cos()` - `tan()` - `asin()` - `acos()` - `atan()` - `atan()` - `sinh()` - `cosh()` - `tanh()` However, it lacks the hyperbolic arc (also known as inverse hyperbolic) functions: - `asinh()` - `acosh()` - `atanh()` Implement them by just exposing the C++ Math library, but clamping its values to the closest real defined value. For the cosine, clamp input values lower than 1 to 1. In the case of the tangent, where the limit value is infinite, clamp it to -inf or +inf. References godotengine#78377 Fixes godotengine/godot-proposals#7110
Describe the project you are working on
I was documenting the
_draw()
low level functions at godotengine/godot-docs#7532 and, as part of teaching a real-world problem wanted to implement a catenary shape (a rope shape between 2 points, not by simulating it physically, but numerically), which requires the usage of a hyperbolic arc (or inverse) tangent function:Describe the problem or limitation you are having in your project
Sadly, the function doesn't exist on GDScript, despite being available on C++ & C# Math libaries, and even in GLSL since 2018 godotengine/godot#16794.
Having no other option, I had to implement a workaround in GDScript, like this:
However, this has many issues:
sin()
,cos()
,tan()
,asin()
,acos()
,atan()
,sinh()
,cosh()
,tanh()
, but not the inverse of these last functions. A user of mathematics functions will expect those functions to be available asasinh()
,acosh()
atanh()
if the direct hyperbolic functions already exist and will be confused why not. While I understand GDScript should not have all math functions and (specially given they are builtin and global scope) measure should be used, either it contains all hyperbolic functions or none of them, appearing quite inconsistent in design.Describe the feature / enhancement and how it helps to overcome the problem or limitation
Create and document the implementation of asinh, acosh and atanh in its Math (globalscope) library, so they are available to the users. Once it is done, docs can be updated to use the same implementation for both C# and GDScript, people are no longer confused by the lack of only some of the hyperbolic functions, and users can now use those to solve curves from differential equations, commonly seen on proyects and games in cartesian coordinates.
From Wikipedia:
In fact, they are so common that they were added to the shader language to generate effects- those same effect are needed to do some geometry and drawing transformations.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
Expose those 3 functions are they are implemented on C++. For the period in which they are not defined, clamp their value to the last valid value or INFINITY -INFINITY if the latest value tends to that. This last part is arguable and very open to discussion.
If this enhancement will not be used often, can it be worked around with a few lines of script?
Yes, I added the lines of script- but as I said before, the issue is not that they are not there, the issue is the inconsistency with only being partially implemented. I would be ok with removing all hyperbolic functions, but that would make no sense and would make GDScript inconsistent with C++, C# and its own shader language.
Is there a reason why this should be core and not an add-on in the asset library?
More complex mathematical functions should be on its own library- we should not convert Godot's Math into numpy, but I think these 3 functions SPECIALLY due to the lightness of its implementation and maintenance, should have its place on globalscope. I will myself add all documentation changes needed.
The only issue I can see is that, because non-virtual builtin methods cannot be shadowed, those projects that have implemented those functions on their own to overcome the issue will see an error on their projects. However, while this could mean the the deployment should happen on a major release, I think it is one of those regressions that is "worth it".
The text was updated successfully, but these errors were encountered: