Refactor BRP to allow for 3rd-party transports #15438

LiamGallagher737 · 2024-09-26T06:24:56Z

Objective

Closes #15408 (somewhat)

Solution

Moved the existing HTTP transport to its own module with its own plugin (RemoteHttpPlugin) (disabled on WASM)
Swapped out the smol crate for the smaller crates it re-exports to make it easier to keep out non-wasm code (HTTP transport needs async-io which can't build on WASM)
Added a new public BrpSender resource holding the matching sender for the BrpReceiver' (formally BrpMailbox). This allows other crates to send BrpMessage's to the "mailbox".

Testing

TODO

alice-i-cecile · 2024-09-26T13:29:51Z

I think this is a good idea, but it's a non-trivial maintenance burden due to the addition of another language. Does it make more sense to maintain this out-of-tree for now until the protocol is stabilized further?

LiamGallagher737 · 2024-09-26T19:41:52Z

I could edit this PR to only fix the building on WASM by extracting the HTTP in to its own module and then move the JS bindings in to my own crate.

alice-i-cecile · 2024-09-26T19:50:16Z

I could edit this PR to only fix the building on WASM by extracting the HTTP in to its own module and then move the JS bindings in to my own crate.

Let's do that! That seems like a nice refactor anyways, and will be able to be merged uncontroversially.

LiamGallagher737 · 2024-09-26T20:19:48Z

Do you think the HTTP transport should have its own plugin rather than being so tied in with RemotePlugin? It won't make a difference now but if more transports do get added we will want a way to select only the ones we want.

alice-i-cecile · 2024-09-26T23:13:09Z

Yes, please split it out into its own plugin :)

crates/bevy_remote/src/http.rs

crates/bevy_remote/src/lib.rs

mweatherley · 2024-09-27T11:43:45Z

crates/bevy_remote/src/lib.rs

        // Fetch the handler for the method. If there's no such handler
        // registered, return an error.
        let methods = world.resource::<RemoteMethods>();

        let Some(handler) = methods.0.get(&message.method) else {
-            let _ = message.sender.send_blocking(Err(BrpError {
+            let _ = message.sender.force_send(Err(BrpError {


Can you explain the rationale for the change to force_send?

I think this ensures errors are reported even if the queue is full. This will overwrite regular messages to ensure the error message is always sent. Did I get that right @LiamGallagher737 ?

The previous send_blocking is not available on WASM. Another option is to use try_send but I don't think it is necessary as the channel is written to from in this function and only once. I could add some docs to support this.

mweatherley

Just some minor doc stuff and a small question.

djeedai

Looks good, a few comments but nothing blocking.

djeedai · 2024-09-27T11:41:23Z

crates/bevy_remote/src/http.rs

+
+impl Plugin for RemoteHttpPlugin {
+    fn build(&self, app: &mut App) {
+        app.insert_resource(HostAddress(self.address))


This overwrites any previous value. Should we insert only if the user didn't, to allow them to overwrite the default address and port?

I appreciate you can configure via the methods, but since world resources are "public", I think the more common pattern in Bevy is to let the user manually insert them before a plugin if they want to override any default. @alice-i-cecile any opinion on that?

Yeah, I'd prefer a manual insertion with docs.

djeedai · 2024-09-27T11:45:43Z

crates/bevy_remote/src/http.rs

+
+/// A resource containing the port number that Bevy will listen on.
+///
+/// Currently, changing this while the application is running has no effect; this merely


Related to previous comment, why are we using a resource at all / why make the field public, if this has no effect? It feels users will get confused.

It's also arguable whether we need 2 separate resources for IP and port; we won't use one without the other.

This is a good point, and I think it especially makes sense now that the transport has its own plugin.

Co-authored-by: Matty <[email protected]>

## Objective Closes bevyengine#15408 (somewhat) ## Solution - Moved the existing HTTP transport to its own module with its own plugin (`RemoteHttpPlugin`) (disabled on WASM) - Swapped out the `smol` crate for the smaller crates it re-exports to make it easier to keep out non-wasm code (HTTP transport needs `async-io` which can't build on WASM) - Added a new public `BrpSender` resource holding the matching sender for the `BrpReceiver`' (formally `BrpMailbox`). This allows other crates to send `BrpMessage`'s to the "mailbox". ## Testing TODO --------- Co-authored-by: Matty <[email protected]>

LiamGallagher737 added 7 commits September 26, 2024 18:10

working feature

afcc9af

only enable http transport feature by default

39990a3

fmt

5147005

typo

239153e

feature gate js module

b7eb86e

swap features for target gates

5b0b708

add docs

501891f

LiamGallagher737 marked this pull request as ready for review September 26, 2024 08:25

LiamGallagher737 marked this pull request as draft September 26, 2024 08:26

handle invalid input

785595e

IQuick143 added C-Feature A new feature, making something new possible A-Dev-Tools Tools used to debug Bevy applications. D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes labels Sep 26, 2024

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 26, 2024

BenjaminBrienen added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 26, 2024

LiamGallagher737 added 2 commits September 27, 2024 07:44

push current changes to keep them in history

a4e440a

remote js bindings

ee86d03

LiamGallagher737 changed the title ~~JS Bindings for BRP~~ Refactor BRP to allow for 3rd-party transports Sep 26, 2024

LiamGallagher737 added 2 commits September 27, 2024 07:52

fix wasm

561522b

docs

b969c4f

Merge branch 'main' into brp-js-bindings

2b9b1c3

alice-i-cecile requested a review from mweatherley September 26, 2024 23:13

LiamGallagher737 added 8 commits September 27, 2024 20:46

Merge branch 'main' into brp-js-bindings

11b5303

remote import

23e7a5e

use core over std

ebc57e6

fmt

6fa5e1f

move http module to its own file

a1aa5fd

docs

1fc0c06

typo

74024e8

nicer url (docs)

d89ff3e

LiamGallagher737 marked this pull request as ready for review September 27, 2024 09:51

mweatherley suggested changes Sep 27, 2024

View reviewed changes

djeedai approved these changes Sep 27, 2024

View reviewed changes

mweatherley approved these changes Sep 27, 2024

View reviewed changes

mweatherley added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 27, 2024

alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 27, 2024

alice-i-cecile added this to the 0.15 milestone Sep 27, 2024

Apply suggestions from code review

bc486a7

Co-authored-by: Matty <[email protected]>

alice-i-cecile removed the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Sep 27, 2024

alice-i-cecile enabled auto-merge September 27, 2024 20:05

alice-i-cecile added this pull request to the merge queue Sep 27, 2024

Use SocketAddr and privatize HostSocket resource

a10c32d

auto-merge was automatically disabled September 27, 2024 20:10
Head branch was pushed to by a user without write access

LiamGallagher737 added 2 commits September 28, 2024 08:12

clippy

a2a8deb

fix example

6e88438

Merged via the queue into bevyengine:main with commit 60cf7ca Sep 27, 2024
29 of 30 checks passed

LiamGallagher737 deleted the brp-js-bindings branch September 27, 2024 20:29

notmd mentioned this pull request Sep 28, 2024

Implement streaming for Bevy Remote #15420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor BRP to allow for 3rd-party transports #15438

Refactor BRP to allow for 3rd-party transports #15438

LiamGallagher737 commented Sep 26, 2024 •

edited

Loading

alice-i-cecile commented Sep 26, 2024

LiamGallagher737 commented Sep 26, 2024

alice-i-cecile commented Sep 26, 2024

LiamGallagher737 commented Sep 26, 2024

alice-i-cecile commented Sep 26, 2024

mweatherley Sep 27, 2024

djeedai Sep 27, 2024

LiamGallagher737 Sep 27, 2024

mweatherley left a comment

djeedai left a comment

djeedai Sep 27, 2024

alice-i-cecile Sep 27, 2024

djeedai Sep 27, 2024

mweatherley Sep 27, 2024

Refactor BRP to allow for 3rd-party transports #15438

Refactor BRP to allow for 3rd-party transports #15438

Conversation

LiamGallagher737 commented Sep 26, 2024 • edited Loading

Objective

Solution

Testing

alice-i-cecile commented Sep 26, 2024

LiamGallagher737 commented Sep 26, 2024

alice-i-cecile commented Sep 26, 2024

LiamGallagher737 commented Sep 26, 2024

alice-i-cecile commented Sep 26, 2024

mweatherley Sep 27, 2024

Choose a reason for hiding this comment

djeedai Sep 27, 2024

Choose a reason for hiding this comment

LiamGallagher737 Sep 27, 2024

Choose a reason for hiding this comment

mweatherley left a comment

Choose a reason for hiding this comment

djeedai left a comment

Choose a reason for hiding this comment

djeedai Sep 27, 2024

Choose a reason for hiding this comment

alice-i-cecile Sep 27, 2024

Choose a reason for hiding this comment

djeedai Sep 27, 2024

Choose a reason for hiding this comment

mweatherley Sep 27, 2024

Choose a reason for hiding this comment

LiamGallagher737 commented Sep 26, 2024 •

edited

Loading