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

Use derive to automatically opt in operations #47

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

Conversation

koonpeng
Copy link
Collaborator

New feature implementation

Implemented feature

Allows certain ops like unzip to be automatically opt in if it is eligible.

Implementation description

This is still very experimental, mostly a POC at this point. This PR uses derive to automatically impl a new DiagramMessage trait for structs that are used as request/response in nodes. There is a new register_with_diagram_message (temp name) function that acts like register_node_builder, except it requires the nodes to have req/resp that impls DiagramMessage. In this POC, only unzip is implemented, but in theory we should be able to do it for fork result and maybe split as well (partially), but it probably won't work for something like fork_clone where we need to check if the struct impl Clone.

This is also compatible with existing impl, we can put this behind a feature flag if we don't want to expose this feature by default.

Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng koonpeng marked this pull request as draft January 13, 2025 04:42
@koonpeng koonpeng changed the title Use derive to automatically register operations Use derive to automatically opt in operations Jan 13, 2025
@koonpeng koonpeng requested a review from mxgrey January 13, 2025 04:42
Signed-off-by: Teo Koon Peng <[email protected]>
@koonpeng
Copy link
Collaborator Author

I'm guessing the introducing of MessageRegistry has the idea to move serialization and operation compatibility to message based instead of node based. So instead of registering if nodes are serializable etc, we register messages instead. If that is the case, then I think derive fits even more.

tbh, the current node based serialization is causing extra complexities, it is possible for 2 nodes with the same req/resp to register with different options (e.g. one can serialize, another cannot), so there are situations where we need to disallow serialization even if we can technically do it. A message based registry would also help in #44.

@mxgrey
Copy link
Contributor

mxgrey commented Jan 14, 2025

So instead of registering if nodes are serializable etc, we register messages instead.

Yep this is the intention, but that would've been a bigger refactor than what I was prepared to do for #27 .

I'm fully supportive of moving in that direction, but we'll probably need to rethink the node metadata as well. Maybe the node metadata should contain the typename of the message types (I think &'static str works for this), and then we also have a dictionary of message metadata where the key of each metadata is the typename.

@mxgrey
Copy link
Contributor

mxgrey commented Jan 14, 2025

My concern with the derive approach remains the fact that we can't use it for types like Result or Option or Protos or ROS messages or any struct that was defined independently of bevy_impulse, but we need to support enabling operations for all those types.

I worry that introducing derive for this will be confusing because it creates two different situational ways to do the same thing. It would probably be easier to provide users with direct and consistent instructions on how to get all their operations registered that work for all scenarios.

The only truly nice solution to the problem will be to use min_specialization. In the meantime in my mind it makes more sense to have one way that works for all message types, no matter their origin.

@koonpeng
Copy link
Collaborator Author

I'm fully supportive of moving in that direction, but we'll probably need to rethink the node metadata as well. Maybe the node metadata should contain the typename of the message types (I think &'static str works for this), and then we also have a dictionary of message metadata where the key of each metadata is the typename.

I have considered using some string based typename before, but the mean issue is that std::any does not guarantee a unique typename, so we can't use it as a key. But what we can do is to introduce a custom TypeInfo struct that includes both the typeid and typename. But atm there is no use for typename other than error messages so that is discarded.

My concern with the derive approach remains the fact that we can't use it for types like Result or Option or Protos or ROS messages or any struct that was defined independently of bevy_impulse, but we need to support enabling operations for all those types.

I wouldn't say it is a blocker as serde (and pretty much every derive) has the same problem.

  1. This is very unrealistic for us. but what serde do is to request upstream to support the trait. Because of this limitation, a lot of crates need to support serde even if they don't ever need to do serialization.
  2. You can wrap the type that you need to support in a newtype struct.
#[derive(DiagramMessage)]
struct WrappedMessage(Base);
  1. We can do something like serde remote to allow support for third party structs, but tbh I think making a newtype struct is much easier.
  2. We already have this "derive problem" because we use serde and schemars. serde is popular enough that it is probably already supported, but there is a good chance that schemars is not. At least I don't think ros2 rust supports schemars, so we already can't serialize any third party structs.

I worry that introducing derive for this will be confusing because it creates two different situational ways to do the same thing. It would probably be easier to provide users with direct and consistent instructions on how to get all their operations registered that work for all scenarios.

The only truly nice solution to the problem will be to use min_specialization. In the meantime in my mind it makes more sense to have one way that works for all message types, no matter their origin.

Yeah I agree multiple ways to do things is not good. But we can put this behind a feature flag, and document only the preferred way. clap has the same issue as well, it supports both the derive API and the builder API, it mostly advertise the derive API but the builder API is still there (you could even say the builder API is the "default" as the derive API is behind a feature flag).

I don't think we can use min_specialization for a long time, even if it become stable soon, we won't be able to use it if we still want to support 1.75.

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