Skip to content
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 BasicTypes overload operators #916

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

poirierlouis
Copy link

Add overload operators for Vector3 and Vector4.

  • Includes +, -, *, /, < and <= operations.
  • Add ThrowLuaError in LuaVM to throw when dividing by zero.
  • Formats Lua usertypes in Scripting for easier maintenance.

2024-01-20_CET_basictypes-overload-operators

Allow to throw an error in LuaVM using 'luaL_error'.
Includes +, -, /, *, < and <= operations.
Formats Lua usertypes for easier maintenance.
Copy link
Collaborator

@WSSDude WSSDude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Game already has these operators defined, and I bet/hope they are done in a bit more performant way than what you do here... Unsure if this should be accepted tbh, if I would much rather somehow rewire game functions already handling these as the operators. Am not a big fan of this PR overall to say the least in its current form.

@WSSDude
Copy link
Collaborator

WSSDude commented Jan 20, 2024

I also think that the ovetloads should be added to RED4ext.SDK and used from there, wouldnt add more specializations into CET when the base is that, just makes things more cumbersome to maintain.

@maximegmd
Copy link
Owner

Looks good to me, I don't see how this could be made more performant, the compiler will do the SIMD optimization and calling the game function has a cost.

@WSSDude
Copy link
Collaborator

WSSDude commented Jan 20, 2024

Unsure if this will get much SIMD optimization and unsure what is the cost of building the new object... Considering this was meant to shadow existing game functions anyway, I at least think it would be better to reuse them. They may have optimizations or specific checks we miss etc. (probably not, but you never know until you look at it, which I didnt at least...)

I'll pull my review if you want to merge it but dont find this particularly good personally, reimplementing things and potentially changing behavior...

Also think this should be part of RED4ext but that was another point, not performance-related.

@WSSDude
Copy link
Collaborator

WSSDude commented Jan 20, 2024

Didnt mean to send it... Wanted to add that I would at least change the comparison operators to take into account some form of epsilon if nothing else. Same for other comparisons, is not the best to compare floating points directly like this.

@WSSDude
Copy link
Collaborator

WSSDude commented Jan 20, 2024

Nah, ended up editing my comments anyway to add few points... Too late to apologise for multiple messages...

Dismissed my review, do what you want based on my comments I guess.

@poirierlouis
Copy link
Author

About performance, I might write a tiny benchmark to compare time of execution between native methods and Lua implementation. See if there is a significant difference and, I guess, avoid any assumption on this matter. Maybe even detect different results...

About epsilon comparison, I was wondering if it should be used. Currently, it is not used with operator ==. But as you point out, it might be within native implementation. Hence, it might be better to rely on native implementation only.

About RED4ext.SDK implementation, if it can be directly implemented there and cascade to Lua without much effort, well yes. The though didn't cross my mind when talking about this missing "feature" in Lua. I'm wondering, currently files for Vector3 and Vector4 are generated. Would it be possible to write operators there without messing the code when generating them?

@poirierlouis
Copy link
Author

poirierlouis commented Jan 21, 2024

Here a tiny-benchmark.txt you can rename with .lua extension and try yourself.

First test is most likely too simple regarding floating point precision... However difference between overload operators and call to native functions speak for itself IMO (loop of 100 000 iterations). Of course it is even worse when you get and call global function without storing a reference before running computation. So I didn't bother to let it in attachment. Elapsed time here is in seconds:

2024-01-21_CET_basictypes-overload-operators-tiny-benchmark

Feel free to improve this benchmark if it is too simple to be representative or point out any mistakes.

Edit: I had concerns order of execution between overload/native functions might have some kind of impact. Tried running native then overload, same results.

@poirierlouis
Copy link
Author

See discussion on RED4ext.SDK.

@poirierlouis
Copy link
Author

I'll update this PR after this is done WopsS/RED4ext.SDK#140.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants