Make defining models more convenient

[hannobraun]

The extern "C" stuff required for the dynamic library isn't great. And the way parameters are passed to the model has caused further pain. Ideally, creating a model should require little or no ceremony to satisfy the plugin system. Maybe something like this would be good:

/// Defines the model parameters
///
/// The `derive` makes the parameter parsing just work, so the user doesn't have
/// to think about it. Preferably in a way that moves the bulk of the work to
/// the host, so compile times for the model stay low.
#[derive(fj::Parameters)]
pub struct Spacer {
    pub outer: f64,
    pub inner: f64,
    pub height: f64,
}

impl fj::Model for Spacer {
    /// Defines the model itself
    /// 
    /// Has access to the parameters through `self`. Method is defined in a
    /// trait, to provide type safety.
    fn model(&self) -> fj::Shape {
        // define the model
    }
}

This is just a sketch, not to be taken too seriously. Maybe it's possible, or maybe a #[no_mangle] or something will still be required.

[mdaffin]

If you use proc_macros you can make them a single function with arguments and expand that into whatever you need to meet the C interface required.

#[fj::model]
pub fn model(outer: f64, inner: f64, height: f64) -> fj::Shape { /* ... */ }

A bit more effort to define the proc_macro, but allows you to change the underlying implementation in the future if required. And keeps a nice simple interface for users to use with minimal boiler plate.

[hannobraun]

Thanks for the suggestion, @mdaffin!

Yes, something like that should be possible. The only thing I'm not sure about, is if proc macros can add attributes, so maybe something like #[no_mangle] would still be in there. We'll see one someone gets a chance to explore this more closely.

I actually like your suggested syntax better than my own! It's just simpler and cleaner.

[mdaffin]

Proc macros can do anything. They just take in a token stream and output a possibly different token stream. For instance, the test_case proc macro adds the #[test] attribute the the tests it generates: https://github.com/frondeus/test-case/blob/master/src/test_case.rs#L78

[hannobraun]

Ah, good to know, thanks!

I've used proc macros before, but never to add attributes. I had this worry in the back of my mind about order of execution in the compiler, because proc macros often are attribute, so can they add something that's already been read at point in compilation. But good to know my worries were unfounded!

[hannobraun]

Relevant thoughts: #220 (comment)

[gabsi26]

If you use proc_macros you can make them a single function with arguments and expand that into whatever you need to meet the C interface required.

#[fj::model]
pub fn model(outer: f64, inner: f64, height: f64) -> fj::Shape { /* ... */ }

A bit more effort to define the proc_macro, but allows you to change the underlying implementation in the future if required. And keeps a nice simple interface for users to use with minimal boiler plate.

I played around with procedural macros today and roughly implemented this

use proc_macro::TokenStream;
use quote::quote;
use syn::parse_macro_input;

#[proc_macro_attribute]
pub fn model(default_values: TokenStream, input: TokenStream) -> TokenStream {
    let vals: Vec<String> = default_values
        .into_iter()
        .filter_map(|tree| {
            if let proc_macro::TokenTree::Literal(lit) = tree {
                Some(lit.to_string())
            } else {
                None
            }
        })
        .collect();
    let item = parse_macro_input!(input as syn::ItemFn);

    let inputs = item.sig.inputs;
    let mut names = Vec::new();
    let mut types = Vec::new();
    for f in inputs.iter() {
        if let syn::FnArg::Typed(meep) = f {
            if let syn::Pat::Ident(ident) = *meep.clone().pat {
                names.push(ident.ident);
            }
            if let syn::Type::Path(path) = *meep.clone().ty {
                types.push(path.path.get_ident().unwrap().clone());
            }
        }
    }
    let block = item.block;

    quote! {
            #[no_mangle]
            pub extern "C" fn model(args: &HashMap<String, String>) -> fj::Shape {
                #(let #names: #types = args.get(stringify!(#names)).map(|arg| arg.parse().unwrap()).unwrap_or(#vals.parse::<#types>().unwrap());)*
                #block
            }
        }.into()
}

This allows one to create a model by specifying:

#[fj_proc::model(5, 1.0, 2.0, 1.0)]
pub fn model(num_points: u64, r1: f64, r2: f64, h: f64) -> fj::Shape {
....
}

which will expand to:

#[no_mangle]
pub extern "C" fn model(args: &HashMap<String, String>) -> fj::Shape {
    let num_points: u64 = args
        .get("num_points")
        .map(|arg| arg.parse().unwrap())
        .unwrap_or("5".parse::<u64>().unwrap());

    let r1: f64 = args
        .get("r1")
        .map(|arg| arg.parse().unwrap())
        .unwrap_or("1.0".parse::<f64>().unwrap());

    let r2: f64 = args
        .get("r2")
        .map(|arg| arg.parse().unwrap())
        .unwrap_or("2.0".parse::<f64>().unwrap());

    let h: f64 = args.get("h").map(|arg| arg.parse().unwrap()).unwrap_or("1.0".parse::<f64>().unwrap());

...
}

Do you think this would be a good way to deal with the ergonomics? @hannobraun

[hannobraun]

Thanks for looking into this, @gabsi26!

I think what you show here is definitely a step in the right direction. The one aspect I don't like, is that you can't associate parameters with their default values at a glance. You basically have to count.

I'm not sure what the best solution is, but maybe something like this:

#[fj::model]
pub fn model(params: Params) -> fj::Shape {
    // ...
}

#[derive(fj::Params)]
pub struct Params {
    #[default(5)]
    pub num_points: u64,

    #[default(1.0)]
    pub r1: f64,

    #[default(2.0)]
    pub r2: f64,

    #[default(1.0)]
    pub h: f64,
}

That would be verbose, but explicit.

Or, we can do something similar to what RTIC does, and generate the parameters struct:

#[fj::model(num_points: u64 = 5, r1: f64 = 1.0, r2: f64 = 2.0, h: f64 = 1.0)]
pub fn model(p: model::Params) -> fj::Shape {
    // Access parameters through the generated struct
    for i in 0..p.num_points {
        // ...
    }
}

As I said, not sure, I'm just throwing out idea. And as I also said initially, what you posted initially is definitely a step in the right direction. I'd merge that, if you submitted it as a pull request (pending proper review; haven't looked at the proc macro code in detail).

One thing we have to be careful about though, is the impact on model compile times. Short compile times are important to enable productive modeling.

By the way, you can get syntax highlighting for the code in your comments using this syntax:

``` rust
    fn my_code() {}
```

[mdaffin]

I am not a big fan of wrapping things into a struct just to pass them to a function.

Another couple of options:

#[fj::model(num_points = 5, r1 = 1.0, r2 = 2.0, h = 1.0)]
pub fn model(num_points: u64, r1: f64, r2: f64, h: f64) -> fj::Shape {

Has some duplication, but I still think overall better - does not hide the fields behind a generated struct.

#[fj::model]
pub fn model(
    #[default(5)] num_points: u64,
    #[default(1.0)] r1: f64,
    #[default(2.0)] r2: f64,
    #[default(1.0)] h: f64,
) -> fj::Shape {

If a bit more verbose than the above but also less disjointed.

Or you could just reply on Default trait and Option:

#[fj::model]
pub fn model(
    num_points: Option<u64>,
    r1: Option<f64>,
    r2: Option<f64>,
    h: Option<f64>,
) -> fj::Shape {
    let num_points = num_points.unwrap_or(5);
    let r1 = r1.unwrap_or(1.0);
    let r2 = r2.unwrap_or(2.0);
    let h = h.unwrap_or(1.0);

Though this is just as verbose as a extra struct.

Some things you might also want to consider - do you want to provide hints to the UI for things like min/max values as well?

[hannobraun]

Thank you for the additional suggestions, @mdaffin!

#[fj::model]
pub fn model(
    #[default(5)] num_points: u64,
    #[default(1.0)] r1: f64,
    #[default(2.0)] r2: f64,
    #[default(1.0)] h: f64,
) -> fj::Shape {

I like this one best, although I don't like it better than my "struct with derive" suggestion. I do like it better than my "generated struct" suggestion.

I don't like the other ones, due to the drawbacks you mention.

Some things you might also want to consider - do you want to provide hints to the UI for things like min/max values as well?

Yes! Doesn't need be part of the first iteration, but eventually, there should be enough information so models don't have to worry much about parameter-related error handling. And enough information to generate a nice HTML form, for example (or some custom bit of UI).

---

Your question triggered a new idea: How about, we encode the required metadata as part of the parameter type, not the model function?

Something like this:

#[fj::model]
pub fn model(num_points: NumPoints, r1: InnerRadius, r2: OuterRadius, h: Height) -> fj::Shape {
    // ...
}

#[derive(fj::Param(default = 5, min = 3, max = u64::MAX))]
pub struct NumPoints(u64);

#[derive(fj::Param(default = 1.0, min = 0.1))]
pub struct InnerRadius(f64);

// etc.

There would be implementations of fj::Param for all common types, so the following would be just as valid, but the default parameters would be less useful (basically the Default implementation of the respective type) and not as nice a UI could be generated.

#[fj::model]
pub fn model(num_points: u64, r1: f64, r2: f64, h: f64) -> fj::Shape {
    // ...
}

[hannobraun]

@gabsi26 has implemented two different suggestions in #643, which I'm in the process of merging right now. I don't think this is the end of the journey (and I've left a bunch of comments on that PR), but I think it's an excellent basis for further experimentation and iteration.

[hannobraun]

I'm going to close this issue now. Its purpose is fulfilled, defining models is more convenient.

I have opened two more specific follow-up issues:

• Switch to model-driven model system #804
• Experiment with alternative model definition syntax #805