Upgrade to Bevy 0.15 #221
Upgrade to Bevy 0.15 #221
Conversation
Still not quite working though 🚧
Vrixyz
force-pushed
the
bevy_0.15
branch
2 times, most recently
from
a00cec9
to
335d678
Compare
Vrixyz
force-pushed
the
bevy_0.15
branch
2 times, most recently
from
f64d1af
to
645eb99
Compare
| @@ -75,7 +75,7 @@ pub fn ui_for_value(value: &mut dyn Reflect, ui: &mut egui::Ui, world: &mut Worl | |||
| queue: Some(&mut queue), | |||
| }; | |||
| let mut env = InspectorUi::for_bevy(&type_registry, &mut cx); | |||
| let changed = env.ui_for_reflect(value, ui); | |||
| let changed = env.ui_for_reflect(value.as_partial_reflect_mut(), ui); | |||
There was a problem hiding this comment.
if you changed the signature of ui_for_value to take a &mut dyn PartialReflect, would you still be able to call it like .ui_for_value(val) where val: &mut dyn Reflect without &*val or something like that? 🤔
There was a problem hiding this comment.
wait mmh; is your question a concern about breaking changes ?
- I believe the answer is no, it won´t be possible (so it's a breaking change), I imagine because of
trait upcasting coercion; Tracking issue for dyn upcasting coercion rust-lang/rust#65991.
| // TODO: display error ? Is entering here even possible ? | ||
| continue; |
There was a problem hiding this comment.
uhhh... this is an edge case in an edge case but I think I would just return None here as well, since this for loop should only work if you're multi-editing a bunch of Handles
There was a problem hiding this comment.
maybe a warn! would be interesting 🤔
| @@ -25,14 +26,15 @@ macro_rules! vec_ui_many { | |||
| $( | |||
|
|
|||
| let same = super::iter_all_eq(values.iter_mut().map(|value| { | |||
| projector(*value).downcast_ref::<$ty>().unwrap().$component | |||
| // FIXME: scary to change a macro | |||
There was a problem hiding this comment.
seems good to me, I think it was just a name change of the method
There was a problem hiding this comment.
try_downcast_ref from PartialReflect actually does 2 things: try_as_reflect (new); then downcast_ref (existing code from main).
I think the new behaviour from other places in this PR is to ignore a failure of try_as_reflect
crates/bevy-inspector-egui/src/inspector_options/default_options.rs
Outdated
Show resolved
Hide resolved
| // FIXME: is the id passed here correct? | ||
| let value_changed = | ||
| self.ui_for_reflect_with_options(v.as_mut(), ui, id, options); |
There was a problem hiding this comment.
I'm not sure this requires an id ; it doesn´t seem related to the rest of the data as it's a draft, but I'm not sure.
| value_to_check.borrow(), | ||
| ui, | ||
| // FIXME: is the id passed here correct? | ||
| id.with(i), | ||
| options, |
There was a problem hiding this comment.
I think that's correct ; but I'm not exactly sure, egui docs state that egui tracks widgets frame-to-frame using [Id]s. ; hashset values are not guaranteed to iter over the same order everytime, so we could either:
- provide an id derived from the reflect_value_hash ?
- sort the values (somehow, based on display or hash or smth 🤔).
- both 😈
I think both makes the most sense.
There was a problem hiding this comment.
Hm, I know that if you run
let hashset = todo!();
for val in hashset {
}the iteration order is not defined but I'm pretty sure that for a given instantiation of a hashset, consecutive iteration will have the same order.
let hashset = todo!();
for val in hashset {}
for val in hashset {} // sameThere was a problem hiding this comment.
My worry is more towards:
let hashset = todo!();
for val in hashset {}
for val in hashset {} // same
hashset.insert("new value leading to recomputation of the set inner representation");
for val in hashset {} // not same
I chose to rely mostly on
PartialReflect, even though I imagine in practice everything isReflect; I'm not sure it's possible to rely onReflectexclusively because when we're cloning the value, errors are handled in another function.I imagine we could either unwrap, or bubble the errors up, but I fear that would make a lot of changes for not much benefits ; also, I believe relaxing trait bounds is always interesting.