Switch to model-driven model system

[hannobraun]

Fornjot's current model system is what I've decided to call "host-driven". The host is in control and just calls into the model function, passing it everything it needs (i.e. the parameters). The model function then creates the model and returns it. The host then goes on to do with that whatever it needs to.

This works well enough for now, but I believe it won't hold up in the future:

• It's not great that every model needs to generate its own logic for handling parameters. Checking that the required ones are there, setting default values, and rejecting parameters that the model doesn't use could all be done on the host side. That would require some unnecessarily complicated API for querying model parameters.
• Same, if we want to support a GUI for setting model parameters.
• We will need a closer interaction between kernel and model to support more advanced modeling features, for example referencing an existing face to put a sketch on it.
• What if we want to support a mode in which the model is an application that uses Fornjot as a library? This would be useful to provide customizable models in a form that doesn't require end users to have Fornjot installed (on a website, for example). This would require a whole different API and a whole different way of writing models.
• Once we allow models to export different artifacts (like a sketch for export to DXF, in addition to a 3D model for export to 3MF), there needs to be an additional API to query which functions are there.

I believe that there is a better approach that can support all those use cases more simply:

• The model just has a simple init/main function without parameters.
• The only thing the host does out of its own initiative is to call this init/main function.
• The host provides an API that the model can call.
• Everything in this API would be #[repr(C)]/extern "C", so it works with over the FFI boundary required by the dynamic library, but also to provide flexibility for future uses (WASM, other languages, etc.).
• In an alternative Fornjot-as-library scenario, the same API would be available to the model.
• The init/main function and some of the API calls (e.g. for loading parameters) are generated by the proc macro.
• The fj crate provides a convenient and idiomatic Rust wrapper, for the model code to call.

Credit goes to @Protryon, who has made me aware of those two different approaches and provided crucial input over in the Matrix channel.

Implementation

Here are my thoughts on how an implementation of this issue could go:

1. Create a new crate (maybe fj-host-api) that defines (but doesn't implement) the host API. The whole crate would basically be an extern "C" { ... } block with a number of function definitions inside. See below for my thoughts on the specific API.
2. Depend on this crate in fj-host and implement the API there.
3. Depend on this crate in fj. Because the API is defined in a lightweight crate without the implementation, this shouldn't negatively affect the compile times of models.
4. Update fj-proc to generate the init/main function and calls to the API for loading parameters.
5. Update fj-host to call the new init/main function instead.
6. Update fj-proc to no longer generate the old and now unused model function.

Here's what I imagine the host API could look like:

extern "C" {
    // Functions for loading model parameters. These would be  required for the
    // initial implementation.
    pub fn load_param_u32(name: *const c_char, default: u32) -> u32;
    pub fn load_param_f64(name: *const c_char, default: f64) -> f64;
    // Add other `load_param` functions for other types as required.

    // API for registering the 3D shape that the model exports. This could
    // probably be left for a later step. The initial implementation could keep
    // the return value from the `init`/`main` function.
    pub fn register_solid(solid: Shape3d);
}

// For the `register_solid` function, we'd need to move this here from the `fj`
// crate. `fj` could just re-export it.
#[repr(C)]
pub enum Shape3d {
    // ...
}

That's just an initial sketch, of course. I'm sure I'm missing something, and I'm sure there are better ways to do it.

[Protryon]

This looks great! As a thought, I like the idea of defining the standard interface as a C header then implementing that as the host API, as opposed to Rust extern "C" blocks defining the host API. The reason is that it allows non-Rust language implementations to have a clearer standard, even though the end result for Rust is identical. rust-bindgen could also be used to generate the bindings for Rust API in fj

[hannobraun]

Thanks for the feedback, @Protryon!

As a thought, I like the idea of defining the standard interface as a C header then implementing that as the host API, as opposed to Rust extern "C" blocks defining the host API. The reason is that it allows non-Rust language implementations to have a clearer standard, even though the end result for Rust is identical. rust-bindgen could also be used to generate the bindings for Rust API in fj

I'm open to that, but let me make a counter-point, in the interest of coming up with the best possible solution.

The fj crate is currently heavily based on enums, and I expect that we will probably want to keep it that way, at least in some cases. Those enums are #[repr(C)], which the compiler accepts without complaint. Hence I assume there must be a reasonable (well, reasonable for a low-level FFI API) definition in C that would be equivalent.

That means in theory, we could generate a C header file from the Rust code, instead of the other way around. This would mean we can keep using enums in Rust, without having to create some complex wrapper code, while still getting the advantages of having a C header. Since Rust is the primary modeling language of Fornjot, and will likely remain that for the foreseeable future, favoring Rust in this way seems like a worthwhile trade-off.

In practice, I don't know if a tool that can generate C headers from extern "C"/#[repr(C)] Rust code exists, but I wouldn't be surprised if it did. Honestly, I would be more surprised if such a tool didn't exist.

[hannobraun]

Ah, addendum to my previous post: My unspoken assumption is, that if we define this enum-equivalent thing in the C header, then rust-bindgen wouldn't be smart enough to make a Rust enum out of that in the generated code. That assumption might be wrong. If it is wrong, then it would barely matter which direction the code generation goes.

[Michael-F-Bryan]

Quoting my comment on #71 about the overall architecture:

I like the init() approach. It gives each model an opportunity to ask the host about things and register interest in certain events while also leaving room for evolution. Just be aware that the "chattier" your guest-host interface, the more painful it'll be to maintain a hand-written extern "C" API.

This is probably more relevant to #804, but one suggestion is to give model developers a "seam" they can use during testing instead of APIs that talk to the host directly. It's hard to explain in words, so here's some code. In our second iteration, we made the mistake of creating free functions for talking with the host (getting arguments, etc.) and as a result it became hard to write unit tests for those interactions. Writing tests that transitively call these extern "C" functions will lead to linker errors, so we ended up only testing the core logic and the code at the periphery like reading arguments and validating inputs - often where the majority of your bugs are - was left untested.

[hannobraun]

Just to mirror my comment from #71, I'd like to thank you for posting this, @Michael-F-Bryan, and say that I'm fully on board with what you're suggesting. A Host trait sounds like a great idea.

[Michael-F-Bryan]

I've put together a proof-of-concept for how we could set up the host-model interface.

There are two main differences between my interface and the existing interface,

1. The host calls a generic init() function and lets the guest register models (or shortcut handlers or whatever), and
2. The guest gives the host extra information about itself and the registered model

Here is my version of the cuboid model:

// TODO: replace this with a custom attribute.
fj_plugins::register_plugin!(|host| {
    let _span = tracing::info_span!("init").entered();

    host.register_model(Cuboid);
    tracing::info!("Registered cuboid");

    Ok(
        PluginMetadata::new(env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION"))
            .set_short_description(env!("CARGO_PKG_DESCRIPTION"))
            .set_repository(env!("CARGO_PKG_REPOSITORY"))
            .set_homepage(env!("CARGO_PKG_HOMEPAGE"))
            .set_license(env!("CARGO_PKG_LICENSE"))
            .set_description(include_str!("../README.md")),
    )
});

#[derive(Debug, Clone, PartialEq)]
pub struct Cuboid;

impl Model for Cuboid {
    fn metadata(&self) -> ModelMetadata {
        ModelMetadata::new("Cuboid")
            .with_argument(ArgumentMetadata::new("x").with_default_value("3.0"))
            .with_argument(ArgumentMetadata::new("y").with_default_value("2.0"))
            .with_argument(ArgumentMetadata::new("z").with_default_value("1.0"))
    }

    #[tracing::instrument(skip_all)]
    fn shape(&self, ctx: &dyn Context) -> Result<fj::Shape, fj_plugins::Error> {
        let x: f64 = ctx.parse_optional_argument("x")?.unwrap_or(3.0);
        let y: f64 = ctx.parse_optional_argument("y")?.unwrap_or(2.0);
        let z: f64 = ctx.parse_optional_argument("z")?.unwrap_or(1.0);
        tracing::debug!(x, y, z, "Creating a cuboid model");

        let rectangle = fj::Sketch::from_points(vec![
            [-x / 2., -y / 2.],
            [x / 2., -y / 2.],
            [x / 2., y / 2.],
            [-x / 2., y / 2.],
        ])
        .with_color([100, 255, 0, 200]);

        let cuboid = fj::Sweep::from_path(rectangle.into(), [0., 0., z]);

        Ok(cuboid.into())
    }
}

To help iterate on design, I've added a polyfill to that repo so you can compile the model to a *.so and load it in Fornjot as-is.

[hannobraun]

Thank you, @Michael-F-Bryan! Looks promising. I'll take a closer look next week.

[hannobraun]

Hey, @Michael-F-Bryan, I've finally had a chance to take a closer look. I'm feeling a bit overwhelmed with all the details, but I'm pretty sure I understand what's going one from a high level.

I think this looks great! The question I'm asking myself though, is what is the next step? A proof of concept is good, but a pull request that I can merge would be much better!

The one concern I have about this, is that the approach taken here is a bit too... professional. We don't have a large user base that expects their models to still work in the next release. So I don't think we need a shim to keep them working, for example. What we do need are specific steps to get improvements like model metadata and WASM support merged. We can keep iterating on the design within the repository.

[Michael-F-Bryan]

Yeah, there's definitely a lot going on in that repo, so I'll try to summarize the main differences from how Fornjot works today.

First, each plugin¹ has some sort of initialization function where it is given a &mut dyn Host and it is able to register the model (do we want more than one model?). This registration process may fail², and it returns some metadata about the plugin like its name and a homepage URL.

// models/cuboid/src/lib.rs

fj_plugins::register_plugin!(|host| {
    host.register_model(Cuboid::default());

    Ok(
        PluginMetadata::new(env!("CARGO_PKG_NAME"), env!("CARGO_PKG_VERSION"))
            // everything after this is optional
            .set_short_description(env!("CARGO_PKG_DESCRIPTION"))
            .set_repository(env!("CARGO_PKG_REPOSITORY"))
            .set_homepage(env!("CARGO_PKG_HOMEPAGE"))
            .set_license(env!("CARGO_PKG_LICENSE"))
            .set_description(include_str!("../README.md")),
    )
});

The other difference is that a model is an object implementing the Model trait instead of a standard function.

// crates/plugins/src/model.rs

/// A model.
pub trait Model: Send + Sync {
    /// Calculate this model's concrete geometry.
    fn shape(&self, ctx: &dyn Context) -> Result<fj::Shape, Error>;

    /// Get metadata for the model.
    fn metadata(&self) -> ModelMetadata;
}

The shape() function is almost exactly identical to our original extern "C" fn model(args: &HashMap<String, String>) -> fj::Shape function, except the HashMap is hidden behind a &dyn Context and we can return errors instead of panicking across the FFI boundary (which is UB 😅).

In theory, it is trival for the #[fj::model] attribute to convert a function into a struct implementing the Model trait. The big benefit we get here is that because there's an object, you now have a place where you can share state³.

The Context is a model's interface to the host, and in theory it's how you could do things like referencing existing faces in the future.

// crates/plugins/src/context.rs

pub trait Context {
    /// The arguments dictionary associated with this [`Context`].
    fn arguments(&self) -> &HashMap<String, String>;
}

Note: The decision to use trait objects instead of concrete types is deliberate - it lets model authors mock out Fornjot's APIs during testing and also leaves space for alternate hosts (imagine a CLI tool that inspects a *.wasm file and lets you know what it contains).

The ModelMetadata tells the host which arguments a Model is expecting, which lets you add the corresponding fields to the UI (plus it has things like the argument's default value and a description that could be used as a tooltip).

The actual guest-host interface is described by the *.wit files in the wit-files/ folder. Functions the host provides to the guest (mainly boring things like logging and aborting) are defined in wit-files/host.wit, and the functions a guest exposes to the host are defined in wit-files/guest.wit.

The *.wit files don't exactly line up with the API model authors see, but that's mainly because I'm trying to work within the limitations currently imposed by wit-bindgen. I would consider the *.wit files to be implementation details of the fj-plugins and fj-host crates.

Another thing you might have noticed is that there's a lot of impl From<guest::Shape> for fj::Shape code. The wit-bindgen tool generates its own version of our Shape struct and does some magic when passing it from guest to host - similar to what fj::Shape's #[repr(C)] annotation does today - so we need to do some conversion when we pass a fj::Shape across the WebAssembly boundary. This adds a bit more boilerplate, but it's better than making the kernel depend on types generated by wit-bindgen⁴.

---

The one concern I have about this, is that the approach taken here is a bit too... professional. We don't have a large user base that expects their models to still work in the next release. So I don't think we need a shim to keep them working, for example.

To be honest, the shim was less for backwards compatibility and more so I could load WebAssembly models while using a stock fj binary 😅

The question I'm asking myself though, is what is the next step? A proof of concept is good, but a pull request that I can merge would be much better!

To integrate this into Fornjot, we would need to:

1. Copy the fj-plugins crate into the Fornjot repo's crates/ folder
2. Update the fj-host to compile to wasm32-unknown-unknown and load models with wasmtime instead of dlopen (the fj_wasm_shim::Plugin type is essentially fj_host::Model, minus the code for automatically recompiling+reloading models)
3. Update the example models to use fj-plugins and make sure they compile
4. Delete the fj_plugins::abi::native module

There is one concern I have with merging such a PR, though... The wit-bindgen project isn't published to crates.io and is only available as a git dependency, which in turn means any Fornjot crates which depend on it (i.e. fj-app via fj-host, and fj-plugins) can't be published to crates.io.

While they would like to publish wit-bindgen to crates.io eventually (bytecodealliance/wit-bindgen#180 (comment)), the priority is to iterate on the component model proposal without worrying the burden of maintenance or backwards compatibility. It depends on the project's priorities, but we may want to wait until wit-bindgen is more mature before making the move to WebAssembly.

That doesn't mean we can't introduce this new API design now, though! I've tried to structure things so that wit-bindgen and WebAssembly are just implementation details and could be swapped in at a later date without anyone noticing.

Footnotes

1. I'm switching to the word "plugin", because this mechanism could be used for more than just implementing a single model. ↩

2. I'm happy to change this. It was mainly added so my shim could return an error when it can't load a WebAssembly file. ↩

3. The use case I had in mind was using salsa to only recompute the bits of my model that changed. That way I can avoid expensive maths or shape generation. ↩

4. We'd actually be using the wit_bindgen_wasmtime::import!() macro to generate our Shape type, so it means compiling the kernel would actually pull in a WebAssembly runtime. ↩

[hannobraun]

Thank you for the summary, @Michael-F-Bryan! Very helpful.

First, a note on nomenclature:

I'm switching to the word "plugin", because this mechanism could be used for more than just implementing a single model.

I think "plugin" is fine for the purpose of this discussion, but I'd like to avoid that word as a user-facing concept, for the following reasons:

• It can be misleading. When reading "plugin", it's reasonable to assume some kind of extension that adds features to Fornjot, instead of a Fornjot model.
• I'd like to support feature extension plugins at some point, which would exacerbate the confusion. This is tracked on the feature wishlist using the term "add-on" (for now).

I suggest staying with model, but saying that a model exports a shape, which can be a solid (3D) or a sketch (2D). In the future, a model will be able to export multiple shapes. In the farther future, models will be able to export other types of artifacts (like assemblies, G-Code, simulation results, ...).

First, each plugin1 has some sort of initialization function where it is given a &mut dyn Host and it is able to register the model (do we want more than one model?). This registration process may fail

I'm happy to change this. It was mainly added so my shim could return an error when it can't load a WebAssembly file.

I don't have a strong opinion on this, but I think fallible registration is fine. We're probably going to need it eventually anyway.

The other difference is that a model is an object implementing the Model trait instead of a standard function.

// crates/plugins/src/model.rs

/// A model.
pub trait Model: Send + Sync {
    /// Calculate this model's concrete geometry.
    fn shape(&self, ctx: &dyn Context) -> Result<fj::Shape, Error>;

    /// Get metadata for the model.
    fn metadata(&self) -> ModelMetadata;
}

The shape() function is almost exactly identical to our original extern "C" fn model(args: &HashMap<String, String>) -> fj::Shape function, except the HashMap is hidden behind a &dyn Context and we can return errors instead of panicking across the FFI boundary (which is UB 😅).

Good to know about the UB! 😄

We should probably catch all model panics and convert them into errors before they cross the FFI boundary. I've made a note to open an issue about that later.

In theory, it is trival for the #[fj::model] attribute to convert a function into a struct implementing the Model trait. The big benefit we get here is that because there's an object, you now have a place where you can share state3.

It might be best to take it one step at a time, and just adapt #[fj::model] to the new implementation, leaving the syntax the same. Eventually, we can extend the syntax, as we extend the capabilities (i.e. models exporting multiple shapes).

3. The use case I had in mind was using salsa to only recompute the bits of my model that changed. That way I can avoid expensive maths or shape generation.

Sounds interesting!

The Context is a model's interface to the host, and in theory it's how you could do things like referencing existing faces in the future.

Could Context be merged into Host? Not saying that it should. Just wondering, why/if we need two different traits for the model to talk to the host.

Note: The decision to use trait objects instead of concrete types is deliberate - it lets model authors mock out Fornjot's APIs during testing and also leaves space for alternate hosts (imagine a CLI tool that inspects a *.wasm file and lets you know what it contains).

Sounds good 👍

The ModelMetadata tells the host which arguments a Model is expecting, which lets you add the corresponding fields to the UI (plus it has things like the argument's default value and a description that could be used as a tooltip).

I think that's going to be an essential capability going forward. The immediate use case being model parameters (which we already have an open issue and draft PR for), but all that other metadata will also be useful down the line.

The actual guest-host interface is described by the *.wit files in the wit-files/ folder. Functions the host provides to the guest (mainly boring things like logging and aborting) are defined in wit-files/host.wit, and the functions a guest exposes to the host are defined in wit-files/guest.wit.

The *.wit files don't exactly line up with the API model authors see, but that's mainly because I'm trying to work within the limitations currently imposed by wit-bindgen. I would consider the *.wit files to be implementation details of the fj-plugins and fj-host crates.

Another thing you might have noticed is that there's a lot of impl From<guest::Shape> for fj::Shape code. The wit-bindgen tool generates its own version of our Shape struct and does some magic when passing it from guest to host - similar to what fj::Shape's #[repr(C)] annotation does today - so we need to do some conversion when we pass a fj::Shape across the WebAssembly boundary. This adds a bit more boilerplate, but it's better than making the kernel depend on types generated by wit-bindgen4.

Maybe I'm wrong, but I think it could be fine to extend the scope of the WIT-generated API a bit. The kernel certainly shouldn't deal with that, but I don't see an immediate reason why the fj-operations crate should be shielded. Its whole purpose is to serve as an interface between the model-generated data and the kernel, so I think it might be appropriate that it knows about the implementation details of that model-generated data.

To be honest, the shim was less for backwards compatibility and more so I could load WebAssembly models while using a stock fj binary 😅

Sounds good! I was expressing concern that you were putting more work into some aspects of this prototype than would be necessary to get something merged. But I'm certainly not against you implementing whatever infrastructure helps you in your work!

To integrate this into Fornjot, we would need to:

1. Copy the `fj-plugins` crate into the Fornjot repo's `crates/` folder

2. Update the `fj-host` to compile to `wasm32-unknown-unknown` and load models with `wasmtime` instead of `dlopen` (the `fj_wasm_shim::Plugin` type is essentially `fj_host::Model`, minus the code for automatically recompiling+reloading models)

3. Update the example models to use `fj-plugins` and make sure they compile

4. Delete the `fj_plugins::abi::native` module

There is one concern I have with merging such a PR, though... The wit-bindgen project isn't published to crates.io and is only available as a git dependency, which in turn means any Fornjot crates which depend on it (i.e. fj-app via fj-host, and fj-plugins) can't be published to crates.io.

Given the state of wit-bindgen, I think it would be better to take the incremental approach and get the non-WASM changes merged first. I don't want to stop publishing those crates.

While they would like to publish wit-bindgen to crates.io eventually (bytecodealliance/wit-bindgen#180 (comment)), the priority is to iterate on the component model proposal without worrying the burden of maintenance or backwards compatibility. It depends on the project's priorities, but we may want to wait until wit-bindgen is more mature before making the move to WebAssembly.

If we decide that we want to go for web support before wit-bindgen is ready (which would mostly be a function of available resources; while I'm busy with kernel stuff, that would mean willingness of contributors to make it happen), we can publish our own temporary fork to crates.io (as fj-wit-bindgen or something).

That doesn't mean we can't introduce this new API design now, though! I've tried to structure things so that wit-bindgen and WebAssembly are just implementation details and could be swapped in at a later date without anyone noticing.

Sounds like the way to go! Let's get the new API design in (which would address this issue) and figure out what to do about WebAssembly from there.

[hannobraun]

This has been addressed in #885. Closing!