Conversation
|
Oh right, |
|
Thanks a lot for adding this! 🚀 Regarding the API, I think it's important that it's consistent with other attributes that defer to functions. At the moment, this would mostly be getters and setters for properties: #[var(set = my_setter)]
field: i64,Thus some comments in that regard:
Sorry if this means that some changes are required, but to avoid this, it would be necessary to discuss the design before implementation 🙂 (although I agree that certain things may only come up during implementation, and a PoC can be a good base) |
6006669 to
33964b5
Compare
- Second, more refined implementation which uses PhantomVar<Callable> instead of directly exposing&keeping the Callable.
33964b5 to
983e517
Compare
Oh yeah, this whole Callable shenanigans turned out to be ultra silly - Rewriting everything took me only a while, so not that much time has been lost. |
Bromeon
left a comment
There was a problem hiding this comment.
Thanks a lot! I'll try to have another look at the API before v0.5 release, but for now I'd say it looks good to be merged, also to avoid divergence with other bigger changes 🙂
What problem does it PR solve?
Adds
#[export_tool_button]– ability to create clickable button in the editor inspector to do things.ExportToolButton is a Var; to be more precise - a
PhantomVar<Callable>.Old description
What problem does it PR solve?
Adds
#[export_tool_button]– ability to create clickable button in the editor inspector to do things.I've been using export tool button a lot recently – they are nice and fast way to expose some functionality, without bothering with editor plugin and whatnot, especially when combined with
validate_property.For example I have exposed tool buttons to spawn win conditions - there can be only one win condition per level, so given tool button checks all the children of the levels, remove old win condition, spawn new one, moves it to the top (so it is the very first child) and manages all the connections. Neat, and implementing it took me less time than writing this paragraph (objectively rust is easier than natural language though. I wish people would talk C irl 😔)!
example - with init!
.. #[var( usage_flags = [EDITOR], hint = TOOL_BUTTON, hint_string = "Win after achieving given score." )] win_at_score: Callable, #[var( usage_flags = [EDITOR], hint = TOOL_BUTTON, hint_string = "Spawn exit after achieving given score." )] spawn_exit: Callable, .. fn init(base: Base<Self::Base>) -> Self { let that = base.to_init_gd(); let win_at_score = Self::create_win_condition_callable(&that, "WinAtScore", &["WinAtScore", "SpawnExit"]); let spawn_exit = Self::create_win_condition_callable(&that, "SpawnExit", &["WinAtScore", "SpawnExit"]); Self { bounds: Default::default(), current_score: Default::default(), level_stats: Default::default(), spawn_exit, win_at_score, base, } }I have two strategies of creating callables for tool buttons - either in init (if init is "trivial") or in POSTINIT/Enter tree:
example - with on_notification
ok, that's all for user story, time for actual meat:
Exposed API and why do we need
THREEFOUR ways to create tool button?!?!?!There are four ways to create export_tool_button. Hold on, I'll explain!
They don't bloat the API at all and every single of them has some specified purpose which can't be (easily) fulfilled by others.
First of all, user can explicitly opt-out from any autogeneration and provide Callable by themselves:
it is for fine-grained control or stuff I haven't thought of earlier.
Theoretically it can be left out, since it is identical to:
Secondly user can auto-generate export tool button to some static fn - not much surprises here. Objectively it is the least useful export_tool_button, but it is still nice for creating pop-ups, emitting signals, interacting with editor plugins etc.
Then we have
Callable::from_object_method. It allows to call methods on base, but has one weakness – it can't really be shared reliably across various classes/structs.Finally there is
fn_selfwhich is prolly the most useful – we don't care about&mut selftoo much, the whole shmuck is about generic programming and declaring shared functionality... // Run fn with &mut dyn Trait receiver #[export_tool_button(fn_self=dyn_fn_one, icon="2DNodes", name="Run dyn fn")] dyn_fn1: Callable, // Run fn with &mut dyn Trait2 receiver #[export_tool_button(fn_self=dyn_fn_two, icon="2DNodes", name="Run dyn fn 2")] dyn_fn2: Callable, // Run Self::method #[export_tool_button(fn_self=Self::my_fn, icon="Blend", name="run Self::fn")] fn_myself: Callable, // Run fn<T:...>(me: &mut T) {...} #[export_tool_button(fn_self=generic_fn, icon="Blend", name="run generic fn")] fn_generic: Callable, .. impl ExampleTrait for MyClass {} impl ExampleTrait2 for MyClass {} fn dyn_fn_one(_this: &mut dyn ExampleTrait) { godot_print!("Hello from ExampleTrait fn!"); } fn dyn_fn_two(_this: &mut dyn ExampleTrait2) { godot_print!("Hello from ExampleTrait2 fn!"); } fn generic_fn<T>(something: &mut T) where T: GodotClass<Base = Node> + Inherits<Node> + WithBaseField { let mut node = Node::new_alloc(); something.base_mut().add_child(&node); let owner = something.base().get_tree().unwrap().get_edited_scene_root().unwrap(); node.set_owner(&owner); } impl MyClass { fn my_fn(&mut self) { godot_print!("hello from my fn!"); } }All of them together
Implementation details or whatever
First of all – KISS. I started from a little too overcomplicated solution and trimmed it down to a little hacky albeit nice and reliable one.
Despite its name export_tool_button is a
Var, not anExport.I didn't want to mess with init due to postinit problems, and plugging-in to
on_notificationwas hard, so I resorted to brute albeit nice solution – I check if Callable is valid in getter, and if it isn't I'm just generating a new one. IMO it is fairly clear and easy to maintain.It makes getter
&mut self(I might rename it to generate or fetch callable later 🤔) but it ain't big deal IMO.Soundness - export tool button Callables are being recreated upon hot reload, so they will always be sound. One can somehow store reference to old Callable, clone it, share it with everything etc. but it isn't something I've seen, it is not a flow one would expect from this feature, and in such a case they can resort to
#[export_tool_button(method="..", ..)].I also wouldn't get too attached to export tool button in current form, since it is broken for GDScript: godotengine/godot#97834 BUT NOT FOR US 😁 and might be subject of change.