-
Notifications
You must be signed in to change notification settings - Fork 131
Update CMatrix class for SA #284
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
base: master
Are you sure you want to change the base?
Update CMatrix class for SA #284
Conversation
* Corrected misleading member names: `up -> forward` representing the Y axis `at -> up` representing the Z axis * Adeed member functions to convert between matrix and Euler Angles. * Renamed parameter names of angular rotation related member functions. * Added CVector version overloads for some member functions.
…ross all games It's not right for me to do so but sure
plugin_sa/game_sa/CMatrix.cpp
Outdated
| } | ||
|
|
||
| void CMatrix::SetScale(CVector const &scale) { | ||
| SetScale(pos.x, pos.y, pos.z); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
Addessed this incorrect passed parameter in 50621d8
|
What about other game versions? |
* Fixed parameters passed to SetScale overload * renamed Translate related function parameters named "pos" (in conflict with CMatrix member with the same name) into "newPos".
Which part in the changes opposes the attempt? If you mean Euler Angle conversion wrappers are different/non-existent across games, then this simply suggests that CMatrix isn't suited to be shared. Alternatively, I could preserve the current CMatrix blueprint by adding a new derived class |
As said look at recent CVector rework. It abandoned calling in-game code at all in favor of implementing them itself. I had plan to do same with CMatrix but I did not came to it yet (there is still CVector2D to update too). You can give a try doing same thing with CMatrix. |
Not everyone has familiarity across every game. Such mindset puts pressure on collaborators who focus on a specific game, therefore limiting attempts to improve the SDK. Plus, the implementation of CMatrix in GTA SA is a gigantic mess, not sure about the other games, but I would assume the same thing. This pull request is intended to focus on |
plugin_sa/game_sa/CMatrix.h
Outdated
| }; | ||
|
|
||
| struct t_EulerAngleConversionFlags { | ||
| unsigned char swapXAndZ: 1 = true; // if set, treats the X axis as Yaw, Z-axis at Pitch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know C++ has dedicated type for storing true/false values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't really matter either way, a single-bit bitfield resolves the same way as a boolean type. It's a matter of coding discipline at my part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use "coding discipline" already present in the project.
|
Was some AI involved in creation of this? Prefixing types with |
Ofcourse those declarations doesn't exists because those are based on how I perceived the ida pseudo-code. And no, this isn't made by AI either. The choice of naming is just my coding discipline. |
* Applied missing const qualifier to signature of `ConvertToEulerAngles` * Added Test Combination Guide for `t_EulerAngleConversionFlags`
* Renamed parameters of Euler Angle Conversion member functions * Removed Confusing Euler Angle Conversion overloads
plugin_sa/game_sa/CMatrix.h
Outdated
| // The rest of the remaining sequence are yet to discover | ||
| // ... | ||
| } sequence; | ||
| struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is flags struct needed at all? Modern reversed just used enum with values that make sense:
https://github.com/gta-reversed/gta-reversed/blob/master/source/game_sa/Core/Matrix.h#L13
In real use-case person will just want to select one of reasonable options, not spending time configuring each flag one by one in some configuration object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was my planing on declaring the conversion flag type into pure enum when the combinations are completely understood. Which is why I decided for the flags struct to stay the way it is as a safer fallback for advanced users to configure onto.
I wasn't aware about reversed source codes containing the euler conversion flags while was specifying the struct type. It's a bad habit of mine of reversing something myself instead of digging onto existing reversed source codes about it. The source material you've tagged here is identical to my findings and seems simpler to use than the current. Yea I think we should adapt the current onto that.
Plugin_SA: