Movement Components refactor #19
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yuokamoto
reviewed
Oct 26, 2021
| @@ -1,13 +1,14 @@ | |||
| // Copyright 2020-2021 Rapyuta Robotics Co., Ltd. | |||
|
|
|||
| #pragma once | |||
|
|
|||
| #include <random> | |||
| @@ -20,6 +20,11 @@ class RAPYUTASIMULATIONPLUGINS_API ATurtlebotBurger : public ARobotVehicle | |||
| public: | |||
| ATurtlebotBurger(const FObjectInitializer& ObjectInitializer); | |||
|
|
|||
| UPROPERTY(EditAnywhere, BlueprintReadWrite) | |||
| UDifferentialDriveComponent* DifferentialDriveComponent = nullptr; | |||
There was a problem hiding this comment.
can't we use robotMoveComponent in base class?
Tadinu
changed the title
Tadinu
reviewed
Oct 26, 2021
Source/RapyutaSimulationPlugins/Public/Drives/RobotVehicleMovementComponent.h
Outdated
Show resolved
Hide resolved
|
@praphulkallakuri I could see this PR is initiated with a couple of commits related to Lidar balancing and it looks like overlapping with #18. |
praphulkallakuri
force-pushed
the
pk_refactor
branch
from
dadc596 to
71a1955
Compare
praphulkallakuri
changed the title
Tadinu
reviewed
Nov 2, 2021
Source/RapyutaSimulationPlugins/Private/Drives/RobotVehicleMovementComponent.cpp
Outdated
Show resolved
Hide resolved
Tadinu
reviewed
Nov 2, 2021
Source/RapyutaSimulationPlugins/Private/Drives/RobotVehicleMovementComponent.cpp
Outdated
Show resolved
Hide resolved
Tadinu
reviewed
Nov 2, 2021
| @@ -23,7 +23,7 @@ void URobotVehicleMovementComponent::UpdateMovement(float DeltaTime) | |||
| const FQuat OldRotation = UpdatedComponent->GetComponentQuat(); | |||
|
|
|||
| FVector position = UpdatedComponent->ComponentVelocity * DeltaTime; | |||
| FQuat DeltaRotation(FVector::ZAxisVector, AngularVelocity.Z * DeltaTime); | |||
| FQuat DeltaRotation(FVector::ZAxisVector, InversionFactor * AngularVelocity.Z * DeltaTime); | |||
There was a problem hiding this comment.
@yuokamoto not sure why AMRVehicle moves in a different style from TurtleBotBurger tho?
@praphulkallakuri And thus, based on the rationale, we might need a more specific name for InversionFactor I suppose.
|
some resolved comment is not resolved yet. please double check. |
yuokamoto
reviewed
Nov 5, 2021
| UPROPERTY(VisibleAnywhere) | ||
| bool IsInitialized = false; | ||
|
|
||
| protected: |
There was a problem hiding this comment.
whyd did you separated protected?
|
I got following error when play the turtlebot3-UE. Did yout test with turtlebot3 as well? UE4Editor!_start() |
Tadinu
reviewed
Nov 5, 2021
| @@ -46,12 +46,12 @@ void URobotVehicleMovementComponent::InitOdom() | |||
| InitialTransform.SetTranslation(PawnOwner->GetActorLocation()); | |||
| InitialTransform.SetRotation(FQuat(PawnOwner->GetActorRotation())); | |||
|
|
|||
| PreviousTransform = FTransform(FQuat(0,0,0,1), FVector(0,0,0), FVector(1,1,1)); | |||
| PreviousTransform = FTransform(FQuat::Identity, FVector::ZeroVector, FVector::OneVector); | |||
There was a problem hiding this comment.
Sorry, I looked too quickly, this could be actually FTransform::Identity
Tadinu
reviewed
Nov 5, 2021
Comment on lines
19
to
22
| void ATurtlebotBurger::InitializeMoveComponent() | ||
| { | ||
| DifferentialDriveComponent = NewObject<UDifferentialDriveComponent>(this, TEXT("DifferentialDriveComponent")); |
There was a problem hiding this comment.
@praphul Kallakuri For the crash that Yu might have just noted, though ATurtlebotBurger seems to be not using RobotVehicleMoveComponent, you could call Super::InitializeMoveComponent() here to initialize it, since in our parent class ARobotVehicle it is currently hooked up to be initialized.
Suggested change
| void ATurtlebotBurger::InitializeMoveComponent() | |
| { | |
| DifferentialDriveComponent = NewObject<UDifferentialDriveComponent>(this, TEXT("DifferentialDriveComponent")); | |
| void ATurtlebotBurger::InitializeMoveComponent() | |
| { | |
| Super::InitializeMoveComponent(); | |
| DifferentialDriveComponent = NewObject<UDifferentialDriveComponent>(this, TEXT("DifferentialDriveComponent")); |
Call Super::, which initializes RobotVehicleMoveComponent [ATurtlebotBurger] definition: cleaning up
|
A rebase over main branch |
yuokamoto
approved these changes
Dec 13, 2021
Tadinu
pushed a commit
that referenced
this pull request
Jan 5, 2022
* Move components duplication refactored for RobotVehicle class and AMRVehicle class * Add StatePublisher class * Add InversionFactor variable in RobotVehicleMovementComponent Co-authored-by: duc <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactored code in
DifferentialDriveComponentin TB classclass URobotVehicleMovementComponentmaking it base classThanks to @Tadinu for help !!