-
Notifications
You must be signed in to change notification settings - Fork 8
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
Redesign template macro #14
base: main
Are you sure you want to change the base?
Conversation
.flatten() | ||
.collect(); | ||
|
||
commands.entity(*scene_tree).build_children(template); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice how this is building a template on a child of another template. Now that receipts are stored on-entity, you can apply any template to any entity and it will just work (remove any components/children included in the previous template but not the new one).
Co-authored-by: Oliver Maskery <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to update RELEASES.md :) I like these changes, but we should get ready to cut a new major version.
Yep yep. Just random question, how do we feel about a 1.0 release? |
No strong feelings. I suppose this is about as 1.0 as we'll ever get? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome evolution of this very handy macro! Loving the neater syntax and the b!
-macro.
Co-authored-by: Viktor Gustavsson <[email protected]>
I would hold off on a 1.0 for now. I'm planning to test this out in some editor stuff sometime soon hopefully, and might find additional stuff to change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
if let Some(parent_id) = world.get::<Parent>(entity_id).map(|parent| parent.get()) { | ||
observer.watch_entity(parent_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is just obvious to other people (I'm not super familiar with how observers work), but perhaps this could do with a comment briefly explaining why we observe the parent if it's there, and why we fallback to listening to all entities otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I'll be sure to document that. My thinking was, this could let you swap out different global observers by collecting them into a template.
($fragments:ident; @ $block:block ; $( $($sib:tt)+ )? ) => { | ||
$fragments.extend({ $block }); // Extend the fragments with the value of the block. | ||
$(push_item!($fragments; $($sib)*))* // Continue pushing siblings onto the current list. | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
($fragments:ident; @ $block:block ; $( $($sib:tt)+ )? ) => { | |
$fragments.extend({ $block }); // Extend the fragments with the value of the block. | |
$(push_item!($fragments; $($sib)*))* // Continue pushing siblings onto the current list. | |
}; | |
($fragments:ident; @ $splice:expr ; $( $($sib:tt)+ )? ) => { | |
$fragments.extend({ $splice }); // Extend the fragments with the result of the expression. | |
$(push_item!($fragments; $($sib)*))* // Continue pushing siblings onto the current list. | |
}; |
Maybe the brace-requirement could be removed for splices as well for consistency:
@counter(num_sheep, "sheep", Button::Increment, Button::Decrement);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to switch from the work account. Again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, I was still on the fence.
I actually like that there is a separation between entity delimiters and component delimiters. Maybe BSN should do the same 🤷♂️ |
Same. Will keep then. |
@NthTensor is the |
Also, could the |
One more comment: I know cart previously mentioned that formatting would work inside the macro, but in this implementation it does not (it seems?). Are you aware of anything specific that causes formatting not to work? I assume anything inside the macro has to be "valid rust syntax" for formatting to work? edit appologies, I misread. Cart mentioned:
And later mentioned in post-MVP work:
|
@JeanMertz Here's the only description I could find on how
Let's try and see if we can make this template formattable using the above information! template! {
Node { position_type: PositionType::Absolute, ..Default::default() } => [ Node { padding: UiRect::all(Val::px(5.0)), ..default() }; Text("Hello World"); ];
}; The first problem is it is using template! [
Node { position_type: PositionType::Absolute, ..Default::default() } => [ Node { padding: UiRect::all(Val::px(5.0)), ..default() }; Text("Hello World"); ];
]; Still not formattable... In valid rust, the delimiters inside a slice template! [
Node { position_type: PositionType::Absolute, ..Default::default() } => [ Node { padding: UiRect::all(Val::px(5.0)), ..default() }, Text("Hello World"), ],
]; Still no formatting... But wait, template![
Node {
position_type: PositionType::Absolute,
..default()
} > [
Node {
padding: UiRect::all(Val::px(5.0)),
..default()
},
Text("Hello World")
],
]; It works! 🥳 @NthTensor it might make sense to use commas after all. The alternative is to do something like this, in which case each fragment is interpreted as a statement, allowing the formatter to work: template!({
Node {
position_type: PositionType::Absolute,
..default()
} > {
Node {
padding: UiRect::all(Val::px(5.0)),
..default()
};
Text("Hello World");
};
}); Along with changing the Of course we'd also have to figure out ways to make names and splices look like valid rust... |
Oh wow, cool! Unfortunately I think => is one of the only valid operators that can appear after an expression in rust, there's limited options there (the syntax also has to be something that can be parsed to AST before the macro runs. I'll have a look at this tomorrow. |
Riiiight, because it wouldn't be able to tell where the expression ends... Yeah that might get tricky |
I just checked, expressions can only be followed by |
For splices, the For names, it would need to use one of the |
I don't know how you feel about abandoning use syn::{
bracketed,
parse::{Parse, ParseStream},
punctuated::Punctuated,
visit::Visit,
Block, Expr, Macro, Token,
};
pub struct AstTemplate {
pub items: Punctuated<AstItem, Token![;]>,
}
impl Parse for AstTemplate {
fn parse(input: ParseStream) -> syn::Result<Self> {
let items = input.parse_terminated(AstItem::parse, Token![;])?;
Ok(AstTemplate { items })
}
}
pub enum AstName {
Static(String),
Dynamic(Block),
}
pub enum AstItem {
Fragment {
name: Option<AstName>,
expr: Expr,
children: Punctuated<AstItem, Token![;]>,
},
Splice(Block),
}
impl Parse for AstItem {
fn parse(input: ParseStream) -> syn::Result<Self> {
if input.peek(Token![@]) {
input.parse::<Token![@]>()?;
Ok(AstItem::Splice(input.parse()?))
} else {
let (name, expr) = match input.parse()? {
// Static name
Expr::Path(expr_path) if input.peek(Token![:]) => {
let name = expr_path.path.get_ident().unwrap().to_string();
input.parse::<Token![:]>()?;
(Some(AstName::Static(name)), input.parse()?)
}
// Dynamic name
Expr::Block(expr_block) if input.peek(Token![:]) => {
input.parse::<Token![:]>()?;
(Some(AstName::Dynamic(expr_block.block)), input.parse()?)
}
// Anonymous
expr => (None, expr),
};
let children = if input.peek(Token![=]) {
input.parse::<Token![=]>()?;
input.parse::<Token![>]>()?;
let content;
bracketed!(content in input);
content.parse_terminated(AstItem::parse, Token![;])?
} else {
Punctuated::new()
};
Ok(AstItem::Fragment {
name,
expr,
children,
})
}
}
} |
In principle I'd be fine with it. I went with |
I think it's a good idea to stay away from proc macros where not needed. My experience with proc macros and RA is not good... Slow and buggy. But when it gets to a point where trying to figure out the declarative way is an even bigger headache than dealing with RA-issues... ⚖️ |
It’s also worth pointing out that one of the goals is for the scene format to be (roughly?) equal both in rust code and in external files. Formatting using rustfmt in Rust code makes sense, but when you have external |
I've redesigned several aspects of the template macro to incorporate community feedback.
Changes
Prototype
trait has been removed.Fragment
no longer is generic over bundle type.Commands
based api has been replaced with a more flexibleEntityCommands
version.pub
.BoxedBundle
trait andb!()
macro. I've opted forb!(Foo)
overbundle!(Foo)
because it gets tedious to write.Callback
component andon(into_observer_system)
helper for adding observers to templates. Observers are added as children which always observe their parents.This is a breaking change, as the syntax of the macro is not fully backwards compatible. Here is the full list of syntax changes:
=>
symbol is now required between a fragment's bundle and it's list of children.{(Foo, Bar)}
is now just(Foo, Bar)
.Example
The proposed syntax changes:
Conditional bundles:
Observers:
Other Notes
For increased compatibility with bsn, i'm considering swapping the
;
for a,
.