-
Notifications
You must be signed in to change notification settings - Fork 92
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
Switch CRDT Synchronization Algorithm to Latest Modification #548
base: bugfix/tree-ui-alignment
Are you sure you want to change the base?
Conversation
93f80bc
to
5b4abd4
Compare
d29beb4
to
bd090a5
Compare
…he behavior system.
…ere. Need to fix root node types.
// ImGui rendering thread. | ||
ros2BehaviorTree.updatePublication(); | ||
publishFrequencyText.ping(); | ||
ros2BehaviorTree.updateSubscription(); |
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.
Important change, publication and subscription handling both happen before the update, which helps avoid edge cases and simplifies CRDT logic.
ros2BehaviorTreeState.getBehaviorTreeSubscription().getPreviousSequenceID(), | ||
ros2BehaviorTreeState.getBehaviorTreeSubscription().getOutOfOrderCount())); | ||
ros2BehaviorTree.getBehaviorTreeSubscription().getPreviousSequenceID(), | ||
ros2BehaviorTree.getBehaviorTreeSubscription().getOutOfOrderCount())); |
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.
Extracted common code between RDXBehaviorTree and BehaviorTreeExecutor, made possible with the new BehaviorTreeNodeHighLayer class (see later comment).
private final CRDTInfo crdtInfo; | ||
private final MutableLong nextID = new MutableLong(0); | ||
private final LatestTimestampModifiable rootReferenceModification; | ||
private final LatestTimestampModifiable dataModification; |
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.
data & root reference are now separate modifiable CRDT fields. That way the topology is synced separately from the data contained by the tree and nodes.
@@ -36,6 +39,14 @@ public class BehaviorTreeDefinitionRegistry | |||
new RegistryRecord(WaitDurationActionDefinition.class, BehaviorTreeStateMessage.WAIT_DURATION_ACTION), | |||
new RegistryRecord(FootPoseActionDefinition.class, BehaviorTreeStateMessage.FOOT_POSE_ACTION) | |||
}; | |||
private static final Map<Class<?>, RegistryRecord> recordMap = new HashMap<>(); |
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.
Added a map to provide a few useful methods.
*/ | ||
public interface BehaviorTreeNode<T extends BehaviorTreeNode<T>> | ||
public interface BehaviorTreeNode<LT extends BehaviorTreeNode<LT>> |
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.
Was thinking about this T wrong before. Documented it to not forget.
@@ -51,8 +52,12 @@ public BehaviorTreeNodeDefinition(CRDTInfo crdtInfo, WorkspaceResourceDirectory | |||
|
|||
this.saveFileDirectory = saveFileDirectory; | |||
|
|||
childrenModification = new LatestTimestampModifiable(crdtInfo); |
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.
The data modifiable is extended, but the children are kept as a separate modifiable in order to handle topology synchronization separately from data, to improve understandability in the ROS2BehaviorTreeSubscription class.
name = new CRDTBidirectionalString(this, BehaviorTreeDefinitionRegistry.getInitialName(getClass())); | ||
notes = new CRDTBidirectionalString(this, ""); | ||
|
||
setModifierName(crdtInfo.getActorDesignation().name() + ": " + name.getValue()); | ||
childrenModification.setModifierName(crdtInfo.getActorDesignation().name() + ": " + name.getValue() + " children"); |
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.
These greatly help with debugging, which need to be left in so we can debug any issues going forward.
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.
This class was a bit of an "ah ha" moment. The old BehaviorNodeLayer class would also represent the state layer, which did not help anything and only made things more complicated. Now this interface only represents RDX & Executor layers, which makes it a lot more useful for extracting common logic, as the state implementations are shared anyway. Also the number of type parameters went from 4 to 3 which should make everyone happier. And even better, they have solid documentation.
*/ | ||
@SuppressWarnings({"rawtypes", "unchecked"}) |
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.
No longer have warnings with the new parameterizations!
@@ -33,6 +26,7 @@ public static void clearLists(BehaviorTreeStateMessage treeStateMessage) | |||
{ | |||
treeStateMessage.getBehaviorTreeTypes().resetQuick(); | |||
treeStateMessage.getBehaviorTreeIndices().clear(); | |||
treeStateMessage.getPartialDataNodes().clear(); |
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.
Partial data is now stored separately in the state messages, no longer exploiting the basic nodes field which was confusing.
{ | ||
rootNodeState.fromMessage(subscriptionNode.getBehaviorTreeRootNodeStateMessage()); | ||
} | ||
else if (subscriptionNode.getType() == AI2RNodeDefinition.class && nodeState instanceof AI2RNodeState ai2rNodeState) |
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.
Was able to remove this bit of boilerplate
import java.util.function.Consumer; | ||
|
||
@SuppressWarnings({"unchecked"}) // Sometimes in this class we need to be a little unsafe |
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.
No more warnings :D
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.
Major modifications to the subscription implementation.
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.
Rewrote/simplified this a lot.
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.
Major rework of the topology operation queue with better method names and methods for root node operations.
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.
Rewrote the topology operations almost entirely. Hopefully you'd agree it's more readable and concise now.
@@ -24,7 +24,7 @@ public static void initializeAction(@Nullable BehaviorTreeRootNodeState actionSe | |||
HandPoseActionState.class, | |||
indexOfInsertion, | |||
sideOfNewAction)); | |||
handPoseAction.getState().update(); |
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.
A lot of getState() methods went away because it's already a state
@@ -143,7 +140,7 @@ public void update() | |||
|
|||
for (RobotSide side : RobotSide.values) | |||
{ | |||
state.copyDefinitionToGoalFoostepToGoalTransform(side); | |||
state.copyDefinitionToGoalFootstepToGoalTransform(side); |
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.
Foostep -> Footstep
@@ -122,7 +122,7 @@ else if (definition.getPlannerInitialStanceSide().getValue() == InitialStanceSid | |||
if (!isPreviewPlanner) | |||
state.getLogger().info("Planning footsteps..."); | |||
FootstepPlannerOutput footstepPlannerOutput = footstepPlanner.handleRequest(footstepPlannerRequest, isPreviewPlanner); | |||
FootstepPlan footstepPlan = footstepPlannerOutput.getFootstepPlan(); | |||
FootstepPlan footstepPlan = footstepPlannerOutput == null ? new FootstepPlan() : footstepPlannerOutput.getFootstepPlan(); |
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.
Fix NPE bug
getDefinition().fromMessage(message.getDefinition()); | ||
|
||
super.fromMessage(message.getState()); |
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.
State fromMessage needs to happen after definition fromMessage. This was a mistake which reared it's head in the new implementation.
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.
Storing the type of node so we can use it and know it in the case of partial data being sent. Also separating the children modification from data modification.
byte FOOT_POSE_ACTION = 18 | ||
# Used to minimize bandwidth, nodes that are send | ||
# without their full data. | ||
byte PARTIAL_DATA = 0 |
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.
Partial data is now code 0
@@ -41,6 +48,8 @@ byte[<=1000] behavior_tree_types | |||
# it's type. | |||
uint32[<=1000] behavior_tree_indices | |||
|
|||
behavior_msgs/BasicNodeStateMessage[<=500] partial_data_nodes |
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.
Good to have the limit at 500 since that's going to determine our max tree size.
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.
Message used for CRDT
@@ -94,7 +94,7 @@ public void changeFrame(String parentFrameName, RigidBodyTransform transformToPa | |||
|
|||
public boolean isChildOfWorld() | |||
{ | |||
return referenceFrame.getRootFrame() == ReferenceFrame.getWorldFrame(); | |||
return referenceFrame != null && referenceFrame.getRootFrame() == ReferenceFrame.getWorldFrame(); |
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.
Fix NPE
Future work: