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.

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

send binary message to specific address #222

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gonpombo8
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Sep 30, 2024

Test this pull request

  • The @dcl/protocol package can be tested in scenes by running
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/protocol/branch//dcl-protocol-1.0.0-12826974654.commit-3b9cde0.tgz"

@gonpombo8 gonpombo8 force-pushed the feat/send-comms-to-peer branch from 08384b4 to a849d19 Compare September 30, 2024 22:47
@@ -11,8 +11,14 @@ message SendBinaryRequest {
repeated bytes data = 1;
}

message SendBinaryToAddress {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
message SendBinaryToAddress {
message AddressedBinaryMessage ? {

message SendBinaryResponse {
repeated bytes data = 1;
repeated SendBinaryToAddress messages_to_address = 2;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont like this name, maybe:

  • targeted_messages
  • private_messages
  • targeted_messages
  • address_messages

@gonpombo8 gonpombo8 force-pushed the feat/send-comms-to-peer branch 2 times, most recently from 914a6c3 to 62a2430 Compare September 30, 2024 23:09
}

message SendBinaryToAddress {
repeated bytes data = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is ok that this is an array of data ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be bytes data = 1, not an array of bytes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if you reach the size limit for a single message and you need to send it in chunks

@gonpombo8 gonpombo8 force-pushed the feat/send-comms-to-peer branch from 62a2430 to 796129a Compare January 9, 2025 09:38
@kuruk-mm
Copy link
Member

kuruk-mm commented Jan 10, 2025

I would recommend the following messages:

message SendBinaryRequest {
  bytes data = 1; // broadcasted message
  repeated string addresses = 2; // when set, only forward to these identities
}

message SendBinaryResponse {}

This is based on the following:

https://github.com/livekit/protocol/blob/4aff238b1235618939dd10dd5aae21a776ad1b3c/protobufs/livekit_room.proto#L186C1-L197C2

If addresses is empty, it broadcasts the message for all peers.

I think it makes sense this way. WDYT?

@gonpombo8
Copy link
Contributor Author

I would recommend the following messages:

message SendBinaryRequest {
  bytes data = 1; // broadcasted message
  repeated string addresses = 2; // when set, only forward to these identities
}

message SendBinaryResponse {}

This is based on the following:

https://github.com/livekit/protocol/blob/4aff238b1235618939dd10dd5aae21a776ad1b3c/protobufs/livekit_room.proto#L186C1-L197C2

If addresses is empty, it broadcasts the message for all peers.

I think it makes sense this way. WDYT?

We can't go with this approach the way the SDK is performed.
For every tick we call the sendBinary, and the response is the data that the client is sending to us.
So if you have multiple messages in a tick for different addresses, you need to call this twice, and it may affect the performance or some things on the client side.
In order to modify this the less, i prefer to be able to send multiple messages to differnet peers on a single request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants