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

VehicleWheel can now return the surface it's colliding with. #55723

Merged
merged 1 commit into from
Dec 10, 2021
Merged

VehicleWheel can now return the surface it's colliding with. #55723

merged 1 commit into from
Dec 10, 2021

Conversation

rydergaming
Copy link
Contributor

@rydergaming rydergaming commented Dec 8, 2021 •
edited by pouleyKetchoupp
Loading

As per this proposed change I implemented the functionality to have give the ability to VehicleWheel to return the surface it's colliding with.

In this picture the two Blue and White bodies' ID's are returned by two VehicleWheels to the console.
image

Bugsquad edit: Implements godotengine/godot-proposals#3640

@rydergaming rydergaming requested review from a team as code owners December 8, 2021 11:24
@rydergaming rydergaming mentioned this pull request Dec 8, 2021
Closed
@akien-mga akien-mga added this to the 4.0 milestone Dec 8, 2021
@akien-mga akien-mga requested a review from a team December 8, 2021 11:37
pouleyKetchoupp
pouleyKetchoupp suggested changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Thanks, it generally looks good! I've left some comments to fix and clarify a few things.

scene/3d/vehicle_body_3d.h Outdated Show resolved Hide resolved
scene/3d/vehicle_body_3d.cpp Outdated Show resolved Hide resolved
scene/3d/vehicle_body_3d.cpp Outdated Show resolved Hide resolved
scene/3d/vehicle_body_3d.cpp Outdated Show resolved Hide resolved
scene/3d/vehicle_body_3d.cpp Outdated Show resolved Hide resolved
doc/classes/VehicleWheel3D.xml Outdated Show resolved Hide resolved
doc/classes/VehicleWheel3D.xml Outdated Show resolved Hide resolved
@rydergaming rydergaming requested review from pouleyKetchoupp and removed request for a team December 9, 2021 10:57
@rydergaming
Copy link
Contributor Author

@pouleyKetchoupp I implemented the requested changes.

pouleyKetchoupp
pouleyKetchoupp approved these changes Dec 9, 2021
View reviewed changes
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp 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! Just make sure you squash your commits into one using interactive rebase.

scene/3d/vehicle_body_3d.cpp Outdated
@@ -225,6 +225,10 @@ bool VehicleWheel3D::is_in_contact() const {
return m_raycastInfo.m_isInContact;
}

Node3D *VehicleWheel3D::get_contact_body() const {
return (m_raycastInfo.m_groundObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpicking, but brackets are not needed :)

@pouleyKetchoupp pouleyKetchoupp added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 9, 2021
@rydergaming rydergaming requested review from a team as code owners December 10, 2021 08:34
@akien-mga akien-mga removed request for a team December 10, 2021 10:22
akien-mga
akien-mga reviewed Dec 10, 2021
View reviewed changes
scene/3d/vehicle_body_3d.cpp Outdated
@@ -225,6 +225,10 @@ bool VehicleWheel3D::is_in_contact() const {
return m_raycastInfo.m_isInContact;
}

Node3D *VehicleWheel3D::get_contact_body() const {
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a trailing tab here, the style checks would fail. While fixing it, might be worth addressing https://github.com/godotengine/godot/pull/55723/files#r765866806 too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed these as well.

akien-mga
akien-mga reviewed Dec 10, 2021
View reviewed changes
scene/3d/vehicle_body_3d.cpp Outdated
wheel.m_raycastInfo.m_groundObject = nullptr;
bool col = ss->intersect_ray(ray_params, rr);
Copy link
Member

Choose a reason for hiding this comment

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

Trailing tab here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thank you!

@akien-mga
Copy link
Member

I noticed this PR got a cherrypick. Could I do that as well once it's merged?

Once merged I would typically do the cherry-pick myself, unless it needs to be a dedicated backport due to significantly different codebases (but here it's similar, I'll just have to handle the node renames like Node3D -> Spatial).

@rydergaming rydergaming force-pushed the get-colliding-body-vehiclewheel branch from 258d2eb to 9cc0596 Compare December 10, 2021 10:39
@rydergaming
Copy link
Contributor Author

I noticed this PR got a cherrypick. Could I do that as well once it's merged?

Once merged I would typically do the cherry-pick myself, unless it needs to be a dedicated backport due to significantly different codebases (but here it's similar, I'll just have to handle the node renames like Node3D -> Spatial).

Ah, okay! I'll let you do it then. I'm sorry I'm new to the Godot Development workflow. (I'm also not a big Git mage)

@akien-mga
Copy link
Member

There's one issue left, the method added in the documentation is not following alphabetical ordering: https://github.com/godotengine/godot/runs/4482502518?check_suite_focus=true

For future contributions, you can use godot --doctool to generate the class reference template instead of doing it manually: https://docs.godotengine.org/en/latest/community/contributing/updating_the_class_reference.html

@rydergaming @pouleyKetchoupp
VehicleWheel can now return the surface it's colliding with.
0c35240 
Fixed PR issues.

Update vehicle_body_3d.cpp

Apply suggestions from code review

Co-authored-by: Camille Mohr-Daurat <[email protected]>
@rydergaming rydergaming force-pushed the get-colliding-body-vehiclewheel branch from 9cc0596 to 0c35240 Compare December 10, 2021 12:22
@rydergaming
Copy link
Contributor Author

There's one issue left, the method added in the documentation is not following alphabetical ordering: https://github.com/godotengine/godot/runs/4482502518?check_suite_focus=true

For future contributions, you can use godot --doctool to generate the class reference template instead of doing it manually: https://docs.godotengine.org/en/latest/community/contributing/updating_the_class_reference.html

Thank you! I fixed this issue and uploaded a new version. I'll be more careful next time.

@akien-mga akien-mga merged commit f675b6b into godotengine:master Dec 10, 2021
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga
Copy link
Member

Cherry-picked for 3.5.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@pouleyKetchoupp pouleyKetchoupp pouleyKetchoupp approved these changes

Assignees
No one assigned
Labels
enhancement topic:physics topic:3d
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@rydergaming @akien-mga @pouleyKetchoupp