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.

Sign up for GitHub

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

Jump to bottom

Add missing useModelFront parameter to GodotSharp Basis and Transform #79082

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Add missing useModelFront parameter to GodotSharp Basis and Transform #79082

merged 1 commit into from
Jul 17, 2023

Conversation

geowarin
Copy link
Contributor

@geowarin geowarin commented Jul 5, 2023 •
edited
Loading

On LookAt methods.
Also adds Vector3 Model constants.

These were not added after #76082 was merged.

@geowarin geowarin requested a review from a team as a code owner July 5, 2023 22:34
@akien-mga akien-mga added bug topic:dotnet cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 5, 2023
@akien-mga akien-mga added this to the 4.2 milestone Jul 5, 2023
raulsntos
raulsntos requested changes Jul 6, 2023
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the .NET module and helping keep it in sync with Core. This looks good already, just making a few comments about things that can be improved.

modules/mono/glue/GodotSharp/GodotSharp/Core/Basis.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Transform3D.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Basis.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Core/Transform3D.cs Outdated Show resolved Hide resolved
@geowarin
Copy link
Contributor Author

geowarin commented Jul 6, 2023

@raulsntos Thanks for your review. I did not know that adding optional parameters would break binary compatibility, but it makes sense that it does.

It wasn't clear from your comments if you wanted me to move the compat methods to the Compat class?
That's what I did, tell me if it's alright.

raulsntos
raulsntos reviewed Jul 6, 2023
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

It wasn't clear from your comments if you wanted me to move the compat methods to the Compat class?
That's what I did, tell me if it's alright.

Sorry about that. It's fine either way, we have never added compat methods for non-generated types before so we don't currently have previous precedent. Usually I keep overloads close to each other, so maybe keeping them in their respective types would be better.

modules/mono/glue/GodotSharp/GodotSharp/Compat.cs Outdated Show resolved Hide resolved
modules/mono/glue/GodotSharp/GodotSharp/Compat.cs Outdated Show resolved Hide resolved
@geowarin geowarin force-pushed the master branch 2 times, most recently from f96011e to 82c2afe Compare July 6, 2023 15:47
@geowarin
Copy link
Contributor Author

geowarin commented Jul 6, 2023

@raulsntos Gotcha! Should be good now 😄

raulsntos
raulsntos approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@geowarin
Add missing useModelFront parameter to GodotSharp Basis and Transform
6c6e5c4 
To LookAt methods.
Also adds Vector3 Model constants.

These were not added after godotengine#76082 was merged.
aaronfranke
aaronfranke approved these changes Jul 6, 2023
View reviewed changes
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Looks good to me

@geowarin
Copy link
Contributor Author

geowarin commented Jul 6, 2023

The build error seems unrelated to this change if I'm not mistaken.
What should I do?

@raulsntos raulsntos mentioned this pull request Jul 7, 2023
Merged
@YuriSizov YuriSizov merged commit 9c74f34 into godotengine:master Jul 17, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 17, 2023 •
edited
Loading

Thanks!

Sorry for no reply earlier. You didn't need to do anything, we restarted the build and it has passed.

@geowarin
Copy link
Contributor Author

@YuriSizov Don't worry about it! I really appreciate your work. Thank you ❤️

@YuriSizov YuriSizov changed the title Add missing useModelFront parameter to GodotSharp Basis and Transform Add missing useModelFront parameter to GodotSharp Basis and Transform Jul 19, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfranke aaronfranke aaronfranke approved these changes

@raulsntos raulsntos raulsntos approved these changes

Assignees
No one assigned
Labels
bug topic:dotnet
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
5 participants
@geowarin @YuriSizov @aaronfranke @raulsntos @akien-mga