High level API

[AdrianEddy]

Initial rough idea for the high level API - definitely not it's final form and implementation yet, there's a lot to still implement here, but the basic examples do work already

I moved pf.rs into pf/mod.rs and separated out parameters.rs from it

There's a global Justfile that handles bundle creation, which needs to be included by the user. This would have to be downloaded by users separately from the repo (unless there's a way to know the path of the crate downloaded contents, but I don't think there is)

[AdrianEddy]

@virtualritz Please take a look around here: https://github.com/AdrianEddy/after-effects/tree/master/premiere/src and let me know what do you think about this structure, where every suite has it's own file. I'd want to do the same for the after effects suites, what do you think?

[virtualritz]

[...] let me know what do you think about this structure, where every suite has it's own file.

Makes total sense. I had something like this planned but never got to it. :grin:

[AdrianEddy]

ae::suites::World2 vs ae::WorldSuite2 ? The former is more rusty, but the latter is maybe easier to write and closer to the Adobe API? Thoughts?

[virtualritz]

I'd forget the "Adobe API". Make it as rusty as possible. Would you still have the suites anyway? Wouldn't that not just be impls on World?

Afaik it's the CoSa API anyway, with lots of stuff added by Adobe over the years. :grin:

[AdrianEddy]

You're right, the impl on World way is the best way. We're lucky nobody warned me about the massive size of the Adobe SDK, and now I'm too deep into this to leave haha :D

So, I fully wrapped a few AEGP suites, including documentation which I had to source from both the header files and the adobe docs, while converting it to proper rustdoc with links to items etc.

By that time, I had seen enough of these suites interfaces to realize that simply XSuite to X wrapper is not possible, because many of these suites have functions that operate on different items, and many items (eg layer) are connected to multiple suites. This, and the amount of suites and functions in them, makes creating proper wrappers extremely time consuming.

So the way I choose to go about this is to mindlessly wrap the suites in their entirety, and then (with a helper macro) optionally add a World type which calls the wrapped functions from the suite. I tried to structure it logically both in files and in the interface/docs. So we have aegp::suites::World and aegp::World, but they are both defined in src/argp/suites/world.rs. Please check out the files and let me know what do you think about this approach. I think it's maintainable. I currently only wrapped World and Camera like that, I'll proceed to wrap more. One drawback is that the doc comments need to be repeated, but maybe that's a good thing as they can be slightly modified to better point to "self"

I'm currently focusing just on the AEGP suites, I'll proceed to other suites/items with the same pattern, unless you have any feedback or ideas.

https://github.dev/AdrianEddy/after-effects/blob/master/src/aegp/suites/world.rs and check out all other files in the AEGP suites folder and aegp/mod.rs

The generated docs look quite nice too.

I also made a list of the available vs wrapped suites in the entire SDK in the main README

[virtualritz]

Yeah, that's why I only wrapped the suites I needed for the plugin I was working on. All of it is just a lot. And some suites are niche uses.

One way to look at it is to also just wrap the most used suites and leave the rest to users of this crate that need them to open PRs for.

https://github.dev/AdrianEddy/after-effects/blob/master/src/aegp/suites/world.rs

My first feedback is regarding naming. I wasn't aware of this when I started working on this (I was a Rust n00b) so many names could be oxidized.

I.e PluginID should be PluginId etc. and all getters need the get_ prefix removed.

[AdrianEddy]

about the get_ naming - yeah, but take a look at World, it has size() which calls get_size from the suite. Suite itself is not an object, so get_ prefix is appropriate there, because there's no self, and there are a lot of different methods, operating on different items.

get_ is removed from items' methods, but I think it should stay in suites

[virtualritz]

I saw this, my counter points would be that:

1. WorldHandle is essentially the object, not self. These are getters in the sense of the doc I linked.

2. For someone using this crate, when resorting to using a suite over the World impl for any reason, they should see the same function names (developer exprerience -- DX).

Some more small things on this topic:

I always find the r#type ugly. Bindgen generates type_. Also ugly but less so? Some people use typ or another word alltogether (e.g. kind). That's a detail though. I don't have a good answer, just dumping my brain.

I would have getter names reflect structs they return 1:1 though. I.e. World::pf_effect_world() should be World::effect_world() since it returns an EffectWorld.

I guess I'm saying: be ruthless when oxidizing names. :grin:

Otherwise World looks very nice.

[virtualritz]

I would have getter names reflect structs they return 1:1 though. I.e. World::pf_effect_world() should be World::effect_world() since it returns an EffectWorld.

Pondering this, type should probably be world_type since it returns a WorldType?

[AdrianEddy]

r#type is indeed ugly, I'll go with world_type then, it's not as nice as type(), but I think it's better than .type_(). kind() would be nice, but then kind() returning WorldType is not intuitive. We have to keep in mind that the API will also be comsumed with people already familiar with the C++ SDK, so we shouldn't change the API too much.

Ok I'll remove get_s from suite functions and proceed to oxidize the rest of the SDK.

I'm not planning to wrap more suites than we already have, there's just too many of them. Like you said, we'll leave that to other users to provide PRs if they need them, but nonetheless I want to make a solid foundation with the suites we have.

[virtualritz]

We have to keep in mind that the API will also be comsumed with people already familiar with the C++ SDK, so we shouldn't change the API too much.

I would totally disregard this as far as naming goes. The changes are not radical anyway.

I think mentioning the resp. function from the original C(++) headers in the docs for stuff we did 'considerably' renames should be more than enough.

Searching for that name in the docs will then pop up the Rust equivalent.

[AdrianEddy]

What's the assume! macro for? Can i remove it?

I wrapped the drawbot using the suite + item approach and it turned out quite nice

[virtualritz]

What's the assume! macro for? Can i remove it?

Feel free to remove anything you deem superfluous. We can always bring it back if we need it again. :grin:

[virtualritz]

I wrapped the drawbot using the suite + item approach and it turned out quite nice

Indeed! Looks juicy.

I think we don't have to mirror stuff like AEGP_CameraType_NUM_TYPES. This is basically just a hack to get the number of variants in the enum on the C/C++ side. It is not an actual variant carrying any meaning to a user or developer.

I.e. I don't think we need this on the Rust side.

[AdrianEddy]

We're getting more rusty, yay!

I wrapped the Item, Layer and Footage and with the latest changes, I turned this:

if let Ok(interface_suite) = ae::aegp::suites::PFInterface::new() {
    if let Ok(layer_suite) = ae::aegp::suites::Layer::new() {
        if let Ok(footage_suite) = ae::aegp::suites::Footage::new() {
            let layer = interface_suite.effect_layer(in_data.effect_ref())?;
            let item = layer_suite.layer_source_item(layer)?;
            let footage = footage_suite.main_footage_from_item(item)?;
            let footage_path = footage_suite.footage_path(footage, 0, 0);
            
        }
    }
}

into this

let footage_path = in_data.effect()
                          .layer()?
                          .source_item()?
                          .main_footage()?
                          .path(0, 0)?;

Here's an example of wrapped Item, which uses functions from multiple suites: https://github.com/AdrianEddy/after-effects/blob/master/src/aegp/suites/item.rs#L268-L386

Suites in these item wrappers are acquired lazily, so if you don't call a function which uses a given suite, it won't be acquired. This is also important when some suites are not available (eg. in Premiere) - in this case the function call returns an Err, instead of returning an error in the Item's constructor.

I assume acquiring and releasing suites is very cheap (like just adding a reference), but I don't know that for sure. Do you happen to know if acquiring and releases suites does anything heavy?

All functions in suites are also now able to accept either ae_sys::AEGP_ItemH, ItemHandle or Item, and I believe that's a zero-cost abstraction

One more nice thing is that suites return ItemHandles, but items return Items

[virtualritz]

I assume acquiring and releasing suites is very cheap (like just adding a reference), but I don't know that for sure. Do you happen to know if acquiring and releases suites does anything heavy?

No, it's cheap.

All functions in suites are also now able to accept either ae_sys::AEGP_ItemH, ItemHandle or Item, and I believe that's a zero-cost abstraction.

That is absolutely awesome.

[AdrianEddy]

I'm getting closer to finishing this thing, and today Parameters got a new life.

I basically written everything from scratch. I'm sorry to throw away most of your params code, but hear me out: The current implementation had a few major issues:

1. Memory of labels was leaked in into_raw()
2. It was not possible to mutate the parameters after calling to_param(), because it created a copy, while it should be a reference to union's data
3. param_def_boxed: std::mem::ManuallyDrop<Box<ae_sys::PF_ParamDef>>, is extremely unclear, with zero help from the rust compiler
4. A mix of macros and raw implementations

So, let's start with the basics: memory! It may look like the ParamDef memory management is confusing, but it's actually pretty simple, and it can be clearly and safely represented by this enum:

pub enum Ownership<'a, T: Clone> {
    AfterEffects(&'a T),
    AfterEffectsMut(&'a mut T),
    Rust(T),
}

The only case where we don't own ParamDef memory is the params parameter in EffectMain. Lifetime of that params is also clear, it can only live as long as the EffectMain call. With this, we can immediately convert that *mut *mut PF_ParamDef to a &[*mut PF_ParamDef] slice in EffectMain and let it live until the end of EffectMain. From that, every other function that touches parameters derives both the reference and lifetime, so if you want to get or set a slider value, you have to reference that original params value. This is done here, where eg. FloatSliderDef takes a reference and enforces that it is not stored any longer than the parent ParamDef.

In every other case, we create the ParamDef memory and we have the ownership (Ownership::Rust). If we call checkout, then AE simply fills out the provided memory, and we have to call checkin, but that memory is still Rust's.

What about the label's memory? The string pointer just has to outlive the call to add_param, and since we now have the correct lifetimes and references in all structs, that's not an issue, and is done eg. here

When parameters are checked out, we create an owned Ownership::Rust value, which has to live for as long as you'll want to use the parameters data, before it's checked in on drop. A nice bonus of correct references and lifetimes, is that every call to set_... sets change_flags = CHANGED_VALUE for you

Example:

let mut param = params.get_mut(Params::Color)?; // Param gets checked out here
let mut color = param.as_color_mut()?;

let current_color = color.value();
color.set_value(ae::Pixel8 { ... }); // and `param`'s CHANGED_VALUE flag is set inside this call

// param is checked in on drop

Another problem was that specific ParamDefs were kind of builders, but not really. Since the memory is referenced now, making it a builder at the same time was tricky, but I settled with this design:

params.add(Params::MixChannels, "Mix channels", ae::FloatSliderDef::setup(|f| {
    f.set_valid_min(0.0);
    f.set_slider_min(0.0);
    f.set_valid_max(200.0);
    f.set_slider_max(200.0);
    f.set_value(10.0);
    f.set_default(10.0);
    f.set_precision(1);
    f.set_display_flags(ae::ValueDisplayFlag::PERCENT);
}));

I think it's not bad.

On top of all parameter handling, we have Parameters struct. which is basically a param manager, which maps params defined by a rust enum to int indices for Ae, sets DISK_ID automatically so it's safe to change order or add params in different versions of your plugin, and simplifies adding, getting and checking out params. Mapping HashMap is created in Command::ParamsSetup and reference to it is passed to every other command call

simply calling params.get(MyParams::My) gets you ParamDef, either referenced from EffectMain's params ptr or checked out using param_checkout. You can also call params.get_at(...) and pass time, to check out the param at a different time.

Adding groups (twirly things) is also supported

I'm pretty happy with this design, but as always I'm open to feedback. Check out parameters.rs, define_param_wrapper! macro and Ownership enum

[virtualritz]

Hey @AdrianEddy, isn't this in a state where it would warrant a merge and release? Even if it's not perfect (docs etc.)?

[AdrianEddy]

sorry about the delay, yes it's almost ready, but I had to take care of other things since then. I definitely want to get this finished soon, so if you don't necessarily need it merged right now, I'd wait a few more weeks until I have my plugin fully finished in case this needs any changes.

[valadaptive]

I'm playing around with this API a bit and I see that the Parameters API uses 32-bit MurmurHash to determine parameter IDs.

While the probability of a collision between IDs should still be quite low, it might be better to use a counter to generate IDs or include some sort of collision checking. With 64- or 128-bit IDs, I wouldn't be as concerned, but 32 bits seems a bit low.

[AdrianEddy]

I'm playing around with this API a bit and I see that the Parameters API uses 32-bit MurmurHash to determine parameter IDs.

While the probability of a collision between IDs should still be quite low, it might be better to use a counter to generate IDs or include some sort of collision checking. With 64- or 128-bit IDs, I wouldn't be as concerned, but 32 bits seems a bit low.

We can't use 64-bit because the id field in AE is only 32-bit. We also can't use a counter, because the idea behind this is to set the "disk id" which is saved in the project file, and it should continue to work even if you update the plugin and change parameters. The goal is that unique parameter name = unique disk id, so even if you add or change parameters later, the order doesn't matter.

I agree that there exists small chance of a collision, but I couldn't think of a better solution than this. Other than passing the responsibility for this entirely to the user of this API (which you can do by using set_id in add_customized)

[valadaptive]

Ah, I didn't realize it was possible to pass a custom ID. That'll definitely work for my use case.

[valadaptive]

@AdrianEddy Would you mind enabling the Issues tab on your fork?

[AdrianEddy]

Sorry it took this long but it is now finally ready to merge. I'm not sure if you want to review it though as it's massive 🙂

I've written the documentation for most important parts, but there are still a lot of structs left undocumented. I would have to copy and rewrite the entire Adobe docs which is too much for me. I believe what's documented + examples are enough

I changed the version to 0.2.0, and after merging it's ready to publish to crates.io

[virtualritz]

Thank you so much for this and yeah, no time to review for now. Everything I saw looked awesome and well though-out.

I'm working full time on an in-house node-based 2D/3D compositing system backed by OpenImageIO and 3Delight. In Rust. That part is closed source, unfortunately, but the OIIO bindings are OSS.

Writing an AfterEffects plug-in for fun and no profit or porting my AtomKraft/Ae Rust re-write to this, both activities that would be a nice way to "review" this, are wishful thinking atm. :grin:

[virtualritz]

Everything is published on crates.io. I could not even test any builds as I'm on a Linux box. So kringers fossed.

TODO: make the docs.rs build succeed. I will open an issue for that.