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

Implement Reflect for tuples up to length 12 #1218

Merged
merged 5 commits into from
Jan 8, 2021
Merged

Implement Reflect for tuples up to length 12 #1218

merged 5 commits into from
Jan 8, 2021

Conversation

TehPers
Copy link
Contributor

@TehPers TehPers commented Jan 6, 2021

Implements Reflect and List for tuples with 0-12 elements (#1022). Since traits in the standard library are only implemented for tuples up to length 12, I figured that the same restriction could apply here.

Open questions:

  • Should tuples implement List? serde_json serializes tuples as arrays, so it might make sense to follow that precedent. The downside is that List::push can't be implemented for tuples, so it must panic.
  • Should Reflect::apply ensure that the other value has the same length as the tuple, or is it safe to allow values with fewer elements for a partial apply? Currently, it will only panic if the incoming value has more elements than the tuple supports.

@DJMcNab
Copy link
Member

DJMcNab commented Jan 6, 2021

For the second point, unless a use case comes up, I would expect that requiring the lengths to be the same is a sensible restriction.

@cart
Copy link
Member

cart commented Jan 7, 2021

Should tuples implement List? serde_json serializes tuples as arrays, so it might make sense to follow that precedent. The downside is that List::push can't be implemented for tuples, so it must panic.

Yeah the fact that you can't push to tuples means they aren't a List imo. And given that bevy_reflect aims to be a "rusty" reflection crate, I think it makes sense to add a new Tuple trait to represent tuples (which would also get added to ReflectRef / ReflectMut). Tuple-ness is semantically useful information to have. And serializers can still choose to serialize tuples as lists if that makes sense.

Should Reflect::apply ensure that the other value has the same length as the tuple, or is it safe to allow values with fewer elements for a partial apply? Currently, it will only panic if the incoming value has more elements than the tuple supports.

Tough call. Reflect::apply is intentionally a bit "fuzzy" to allow "patching" scenarios. So we have to decide if it makes sense to "patch" (A, B, C) with (A, B). I think its probably safe to fail in that case for now. I can't think of a scenario where that would be desirable.

@cart
Copy link
Member

cart commented Jan 7, 2021

Yeah the fact that you can't push to tuples means they aren't a List imo

Hmm maybe that isn't true. I would probably impl List for arrays and they can't be pushed to.

@TehPers
Copy link
Contributor Author

TehPers commented Jan 7, 2021

Yeah the fact that you can't push to tuples means they aren't a List imo. And given that bevy_reflect aims to be a "rusty" reflection crate, I think it makes sense to add a new Tuple trait to represent tuples (which would also get added to ReflectRef / ReflectMut). Tuple-ness is semantically useful information to have. And serializers can still choose to serialize tuples as lists if that makes sense.

I agree with this. Semantically, tuples are not lists. Tuples aren't designed as collections the same way that lists, sets, and maps are. That being said, tuples have been shown to hold similarities to lists in dynamic languages (for instance Python). I think that if we were to create a new Tuple trait, we would find that there are a lot of similarities between it and the List trait. Additionally, it would be very similar to how we would handle arrays, which also have a fixed size and are essentially n-tuples of a single type (although not semantically, but in terms of how they are used).

At the very least, if we were to create a Tuple trait, I think that we would need to change some of the interface for it. For instance, you don't "push" to a tuple, but rather set a value in it. It would make sense in that instance to remove the push method and favor get_mut instead for updating the fields. Additionally, it might not make sense to have the iter function on the tuple either. Each field could potentially be a different type, and semantically tuples aren't intended to be iterated over since they are not collections.

I don't mind creating a new Tuple trait with these changes and implementing that instead. It would also work better with serialization since some serializers don't turn tuples into list (for example RON). Do you think we should go this route instead, and if so do you have any suggestions for the functions that should exist on the Tuple trait?

@TehPers
Copy link
Contributor Author

TehPers commented Jan 7, 2021

I noticed there's a TupleStruct trait as well. Could we use this instead of List somehow? The only issue I see is that DynamicTupleStruct requires a name.

@cart
Copy link
Member

cart commented Jan 7, 2021

I think it probably makes sense to use TupleStruct as a base, but I think it should be a new Tuple type (without the name).

I'm also totally fine with adding an iterator over tuple values. While that wouldn't normally make sense in a static language, one of the goals of bevy_reflect is to enable dynamic interactions with data.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Love the changes! I think this is good to go. Just one (very minor) nit.

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
@cart cart merged commit 5e74561 into bevyengine:master Jan 8, 2021
@TehPers TehPers deleted the tehpers/reflect-tuple branch January 8, 2021 07:46
rparrett pushed a commit to rparrett/bevy that referenced this pull request Jan 27, 2021
Add Reflect impls for tuples up to length 12
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.

3 participants