Skip to content

adding OvMaths::FMatrix4::Rotation and OvMaths::FMatrix4::Rotate#130

Merged
adriengivry merged 2 commits intoOverload-Technologies:developfrom
kmqwerty:feature/adding_Rotation_and_Rotate
Sep 17, 2020
Merged

adding OvMaths::FMatrix4::Rotation and OvMaths::FMatrix4::Rotate#130
adriengivry merged 2 commits intoOverload-Technologies:developfrom
kmqwerty:feature/adding_Rotation_and_Rotate

Conversation

@kmqwerty
Copy link
Copy Markdown
Contributor

@kmqwerty kmqwerty commented Sep 16, 2020

Contributes to #92

#include "OvMaths/API/export.h"
#include "OvMaths/FVector3.h"
#include "OvMaths/FVector4.h"
#include "OvMaths/FQuaternion.h"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you forward declare FQuaternion because of circular reference, this include should get moved to the .cpp file.

/**
* Forward declaration due to circular reference
*/
struct FMatrix4;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't necessary as FMatrix4. is already included.
It can be either removed, or we can move #include "OvMaths/FMatrix4.h" from this file to the .cpp file.

@adriengivry
Copy link
Copy Markdown
Member

adriengivry commented Sep 16, 2020

Looks good overall! I've noted a couple of stuff that we should fix.

Morehover, in the future, it would be good to forward declare everything that we can in header files, and move all our includes from header to source files.
This was a bad practice that I wasn't aware of at the time most of Overload code/architecture has been made. As a rule of thumb, never include other header files from a header file, unless you are implementing some inline stuff that requires knowing more than just the symbol.

Thanks for your contribution tho, it's greatly appreciated :)

@adriengivry
Copy link
Copy Markdown
Member

adriengivry commented Sep 16, 2020

One other thing, this doesn't resolves #92 as this issue requires exposing these new methods to lua.

Copy link
Copy Markdown
Member

@adriengivry adriengivry left a comment

Choose a reason for hiding this comment

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

Looks great!

@adriengivry adriengivry merged commit 286b106 into Overload-Technologies:develop Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants