Add groups export. #1214
Add groups export. #1214
Conversation
Yarwin
added
feature
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1214 |
- Edit registered_docs.xml – from now on `export`ed fields are being documented after `#[var]` fields.
|
After change Example:
|
|
Looks like the docs are just using alphabetical order? |
|
Coincidence – "a" is after "b" while all the items are not a it went from: for gdext/itest/rust/src/register_tests/register_docs_test.rs Lines 156 to 179 in e1073bc to I can change this order and document this quirk 🤔 EDIT: ah, now I see – you meant GODOT docs, not the test ones 😅 – it seems you are right (I checked it with other examples and yeah, they are sorted alphabetically). |
- Formatting fixes, use `sort_by` instead of `sort_unstable_by` (as the already created comment states).
|
Properties are implicitly added to the most recently defined group, like in GDScript, right? I'm not a fan of attributes having implicit effects on other properties. |
| @@ -749,6 +737,7 @@ fn parse_fields( | |||
| } | |||
|
|
|||
| Ok(Fields { | |||
| groups, | |||
There was a problem hiding this comment.
Could you call format_groups here and pass the Vec by value? fn format_groups(groups: Vec<String>) -> Vec<String>
| pub(crate) fn new_from_kv(parser: &mut KvParser, groups: &mut Vec<String>) -> Self { | ||
| Self { | ||
| group_name_index: Self::parse_group("group", parser, groups), | ||
| subgroup_name_index: Self::parse_group("subgroup", parser, groups), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This currently accepts any token-tree and just prays that it's a string litteral. This is also accepted:
#[export(group = f64::MAX)]but is turned into a group "f64 :: MAX" which is not something I'd expect.
There was a problem hiding this comment.
I would argue that it might be desirable, bigger issue might be typo such as #[export(group = g1765 range = (0.0, 10.0, or_greater))] (lack of comma) 😄
There was a problem hiding this comment.
fix'd, I also included key with a proper span pointing at the attribute, for example:
error: missing value for 'subgroup' (must be String literal)
--> src/lib.rs:27:14
|
27 | #[export(subgroup = some subgroup 2)]
| ^^^^^^^^
error: value for 'group' must be String literal; found extra Ident { sym: range, span: #0 bytes(803..808) }
--> src/lib.rs:36:14
|
36 | #[export(group = "g1765" range = (0.0, 10.0, or_greater))]
| ^^^^^It works nicely with IDEs, albeit it handles only one error at once 🤔
- Check for string literal, instead of any expression; Return the key with proper span in the errors.
Yeah, I'm not a fan either. I tested gdscript-like syntax and it results in jank – this is why I'm doing all these shenanigans with sorting&checking groups. |
Personally, I would prefer explicitly setting the desired group on every property. That would also make things simpler for you. |
This is extacly what I went for – every property has explicitly set group and subgroup. The problem is with the Godot itself – it assumes that every property registered after given group/subgroup is a part of said group/subgroup – therefore forcing us to sort all the properties by their group/subgroup association. I'm not sure if it can be changed upstream (due to backwards compatibility and whatnot – it has been like that for years). Example, to make it 100% clear: #[derive(GodotClass)]
#[class(base=Node, init)]
pub struct NodeWithGroups {
#[export(group = "my group", subgroup = "my group subgroup")]
property_6: i32,
#[export(group = "my group")]
property_5: GString,
#[export(subgroup = "my subgroup")]
property_3: f32,
#[export]
property_1: i32,
#[export(group = "my group")]
property_4: f32,
#[export(group = "my group 2")]
property_8: i32,
#[export(group = "my group", subgroup = "my group subgroup")]
property_7: i32,
#[export]
property_2: i32,
}and the result: |
|
Excellent work! Small issue: I think subgroup without group makes no sense and should result in a compile error, since it's likely a user error, even if Godot allows it and implicitly converts the subgroup to a non-sub group. |
|
@Yarwin thanks for the clarification. I misunderstood this part when reading the PR the first time. |
Is there any reason for us to support "non-canonical" order, i.e. The sorting logic is definitely nice to have and I wouldn't remove it, but maybe we should for now expect that fields are already declared in the correct order. Meaning: apply sort, check if order changed, if yes emit compile error. Also, will the relative order of properties without group change? Because that wouldn't be good. |
- Prevent declaring subgroup without group.
Note: Firstly I wasn't sold on it, but having subgroup & group with the same name results in jank.
Yep, this way user is free to sort their properties on their rust struct as they wish. It is easy to imagine order such as: // MOVEMENT PROPS
#[export (group="movement") ]
speed: f32,
// NODES
#[init(...)]
animation_tree: OnReady<Gd<AnimationTree>>,
#[export]
camera: OnEditor<Gd<Camera3d>>,
#[export(group="movement")]
character_body: OnEditor<Gd<CustomCharacterController>>,
// NAVREGIONS
#[export(group="navigation_stuff)]
(...)
// CAMERA STUFF
(...)Additionally groups are declared explicitly with the
While we never guaranteed the proper order (albeit it always worked well 😄) relative ordering is being preserved (firstly declared go first etc). |
I get that, but why would anyone do this? The inspector is the user-facing representation of fields, which people likely interact with most (otherwise they wouldn't use Why not maintain that same connection in the Rust source? What's the benefit? Was this feature (splitting declarations belonging to the same group) ever requested in GDScript?
Thanks! Just to be clear, this also applies to any fields within the same group/subgroup, right? Not just on top level. |
- Fix docs.
There was a problem hiding this comment.
Thanks a lot, overall great work 👍
Not fully convinced about per-field attributes + reordering. While per-field is more rusty, it comes with some disadvantages: it needs repetition of both group and subgroup name. This would be less of a problem if the proc-macro didn't require a literal, but with this design it's not possible to store the name in a constant and change in only one place. Accidentally making a typo creates a new group.
GDScript doesn't have these problems.
@export_group("My Properties")
@export var number = 3
@export_subgroup("Extra Properties")
@export var string = ""
@export var flag = falseIt's also considerably less verbose (granted, the init we have to do):
#[export(group = "My Properties"))
#[init(val = 3)]
number: i32
#[export(group = "My Properties", subgroup = "Extra Properties"))
string: GString,
#[export(group = "My Properties", subgroup = "Extra Properties"))
#[init(val = false)]
flag: bool,Not saying we have to change this, but we should at least foresee how we can expand from here, to not design ourselves into a corner. Which also gets us back to my last comment: if we required the correct order in the first place, we could still go both ways in the future:
- Allow reordering of properties without changing grouping (current impl)
- Omit repetition of group/subgroup to infer it from previous field (GDScript impl)
Both would be backwards-compatible. Committing to field reordering today however closes that door for relatively little benefit. We don't have user feedback yet -- if anything, we know from thousands of users that the GDScript way works, but there's zero indication that reordering is a necessity.
| /// Names of all the declared groups and subgroups for this struct. | ||
| // In the future might be split in two (for groups and subgroups) & used to define the priority (order) of said groups. | ||
| // Currently order of declaration declares the group priority (i.e. – groups declared first are shown as the first in the editor). | ||
| // This order is not guaranteed but so far proved to work reliably. |
There was a problem hiding this comment.
We should probably guarantee it.
People use #[export] because they want to have a UI representation, and with that come certain expectations. Changing order would break user workflows for no good reason. At the very least, we should never change this in patch versions.
Maybe we can even test this, if certain APIs return the same order as in inspector.
There was a problem hiding this comment.
Fortunately get_property_list will be enough and will return the same order as one in the inspector.
| // Ungrouped fields or fields with subgroup only always have higher priority (i.e. are displayed on top). | ||
| (Some(_), None) => Ordering::Greater, | ||
| (None, Some(_)) => Ordering::Less, |
There was a problem hiding this comment.
or fields with subgroup only
This is outlawed now, no?
| Some(quote! { | ||
| ::godot::register::private::#register_fn::<#class_name>( | ||
| #group, | ||
| ); | ||
| }) |
| /// To declare groups and subgroups append `group` key to a `#[export]` attribute. Fields without group will be exported first, | ||
| /// followed by properties with group only, tailed by ones with group & subgroup declarations. Relative order of fields is being preserved |
There was a problem hiding this comment.
Relative order of fields is being preserved
Maybe mention that this applies within the same group/subgroup, as well as on top level.
| /// Handles a string literal (`att = "str"`). | ||
| pub fn handle_string(&mut self, key: &str) -> ParseResult<Option<String>> { | ||
| self.handle_literal(key, "String") | ||
| .map(|possible_literal| possible_literal.map(|lit| lit.to_string())) | ||
| } |
There was a problem hiding this comment.
We didn't add this because string properties can generally be expressions.
It might be OK to support only literals in the first spec, but maybe add a comment like
If we support expressions in the future (e.g. names of
consts), this is no longer needed.
|
Maybe instead of supporting non-literals, we could in the future allow a "config" struct for groups? Prior art in #[godot_api]
impl MyStruct {
#[rpc(authority, unreliable_ordered, call_remote, channel = 2)]
fn explicit(&mut self) {}
#[rpc(config = MY_RPC_CONFIG)]
fn external_config_const(&mut self) {}
}
const MY_RPC_CONFIG: RpcConfig = RpcConfig {
rpc_mode: RpcMode::AUTHORITY,
transfer_mode: TransferMode::UNRELIABLE_ORDERED,
call_local: false,
channel: 2,
};Here, this could mean: #[derive(GodotClass)]
struct MyStruct {
#[export(group = "g", subgroup = "sub-g")]
explicit: i32,
#[export(group_config = GROUP)]
external_config_const: i32,
}
const GROUP: GroupConfig = GroupConfig::new("g", "sub-g");Either way, it would need moving the sorting/validation to runtime, but we already do similar things in other places (e.g. calculating values of |
|
In general groups are After re-thinking all of it again, I think I'll go with gdscript implementation 🤔 (and expose the |
|
Another idea would be to avoid inline definitions entirely, and reserve only // Special methods for ease of use.
use godot::register::{group, subgroup};
const MAP: PropertyGroup = group("Map");
const MAP_TILES: PropertyGroup = subgroup("Map", "Tiles");
const MAP_ENEMIES: PropertyGroup = subgroup("Map", "Enemies");
const PLAYER: PropertyGroup = group("Player");
#[derive(GodotClass)]
struct LevelConfig {
#[export] // top-level
name: GString,
#[export(group = MAP)]
terrain_type: Terrain,
#[export(group = MAP_TILES)]
width: u8,
#[export(group = MAP_TILES)]
height: u8,
#[export(group = MAP_ENEMIES)]
enemy_positions: PackedVector2Array,
#[export(group = PLAYER)]
player_hp: u16,
}Still not sure if we should go rather the "rusty" or the "godoty" way... 🤔 |
|
If we go for the Godot "attribute affects all following fields" way, we might want to make this clearer with an attribute other than Maybe even something that underlines the "multi-field" semantics. Being separate from |
Bromeon
force-pushed
the
master
branch
2 times, most recently
from
ea8033b to
db2bdce
Compare
What does this PR solve?
Adds groups and subgroup.
How does it work?
I checked few options and finally went with a
clapAPI (suggested by @ttencate) which requires specifying the group directly inside a#[export]attribute – everything else turned out to be too limiting, too messy or too janky 😒.Example usage
Problems etc
Initially I wanted to ship it with struct extensions but they can be added in separate PR (I'll create the issue for that later). The leftovers of that is
Fieldsstruct being moved to separate file – I don't see anything wrong with that though, so I left it like that (not sure aboutnamed_fieldsfunction though).I experimented with organizing fields into something along the lines of
but it turned out to be silly, unnecessary and unnecessary overcomplicated.
I am not really happy with
GroupIdentifier(usizewhich simply points to some index holding group names) but it is kinda the sanest solution – idents make no sense (and use "to_string()" underneath anyway), strings require too many unnecessary allocations,*const Stringis… uh… let's not do that.