-
Notifications
You must be signed in to change notification settings - Fork 494
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
HITL: server-side helpers to measure client latency #1796
Conversation
…p_recent_server_keyframe_id to help the server measure client latency
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.
Some comments.
message_key | ||
] | ||
# add/update all messages | ||
for message_key in inc_message: |
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.
It seems like a good general rule that consolidating messages simply means updating to the latest message in the latest incremental keyframe. Let's beware a future situation where a client must absolutely receive an entire sequence of messages (not just the latest one).
@@ -48,6 +50,18 @@ | |||
document.getElementById('serverPort').disabled = true; | |||
document.getElementById('sendButton').disabled = false; | |||
ws.send('client ready!'); // Send "client ready!" to the server | |||
|
|||
// Start sending messages at 10 Hz | |||
sendIntervalId = setInterval(function() { |
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.
As part of this PR, the stub HTML client now sends client states at 10 Hz.
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.
LGTM!
@@ -12,6 +12,7 @@ | |||
from habitat_hitl.core.gui_input import GuiInput | |||
|
|||
|
|||
# todo: rename to RemoteClientState |
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.
👍
…p_recent_server_keyframe_id to help the server measure client latency (#1796)
…p_recent_server_keyframe_id to help the server measure client latency (#1796)
…p_recent_server_keyframe_id to help the server measure client latency (facebookresearch#1796)
…p_recent_server_keyframe_id to help the server measure client latency (facebookresearch#1796)
Motivation and Context
Add ClientMessageManager.set_server_keyframe_id and RemoteGuiInput.pop_recent_server_keyframe_id to help the server measure client latency.
This feature will require follow-up work in the Unity client.
stub_client.html
provides a reference implementation.In the example code below, roughly speaking, this is the latency between something happening in a sim_step on the server and the client's reaction (as GuiInput) being available in a later sim_step.
Usage: consider a server AppState that maintains a frame counter. It uses
ClientMessageManager.set_server_keyframe_id
on every frame. The client receives and renders a frame with serverKeyframeID=k. It should includerecentServerKeyframeId=k
in the next clientState it sends to the server. (See stub_client.html for an example.). In the server AppState, it can checkRemoteGuiInput.pop_recent_server_keyframe_id()
and compare it to its current frame counter.Example usage in an AppState:
How Has This Been Tested
Local testing with rearrange and pick_throw_vr AppStates. With stub HTML client. Regression testing with Unity in-editor client (although it doesn't yet implement the required client-side functionality for this feature).
Types of changes
[HITL]
Checklist