bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.15k stars 3.46k forks source link

Document that zero-length reflection paths are allowed. #13459

Open viridia opened 3 months ago

viridia commented 3 months ago

What problem does this solve or what need does it fill?

Working with reflected data types and fields, I often have to treat the outermost type as a special case. It would be helpful to unify the logic for handling fields and their containers.

For example, my reflective property inspector widget uses a set of nested widgets, so if you are editing say a struct containing an option containing an f32, you have three widgets: a struct inspector, an option inspector, and an f32 inspector. However, the logic for the struct inspector has to be different because it's operating at the root level.

What solution would you like?

It would be convenient if the various reflection methods that accept paths would permit a zero-length path. This would operate on the entire struct/tuple/whatever, instead of on a specific field.

What alternative(s) have you considered?

I could do this myself by adding a check for is_empty() on the path, but this would introduce additional clutter in my code.

Additional context

Discord thread: https://discord.com/channels/691052431525675048/1002362493634629796/1242538258211143783

MrGVSV commented 3 months ago

Could you provide a minimum reproducible example? I'm trying this out now and it seems like reflection paths do actually support empty paths:

let mut foo: Option<usize> = Some(123);
assert_eq!(*foo.path::<Option<usize>>("").unwrap(), foo);
viridia commented 3 months ago

Oh, I never actually tried it, I just assumed it wouldn't work, or at least was undefined behavior. We can close the issue, or if you like, update the docs to indicate that this case is explicitly supported.

MrGVSV commented 3 months ago

Oh, I never actually tried it, I just assumed it wouldn't work, or at least was undefined behavior.

Yeah, that was my assumption too 😅

We can close the issue, or if you like, update the docs to indicate that this case is explicitly supported.

I agree we should make this more explicit in the docs. And it would be nice to also have explicit test cases for this as well.

MrGVSV commented 3 months ago

@viridia could you update the issue title to indicate this is now a documentation/testing update?

mweatherley commented 3 months ago

Good to know I'm not crazy 😅 I was looking at the source code and wondering why zero-length paths wouldn't work.