Skip to content

Code Generation from Json and better build infrastructure #35

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

Merged
merged 3 commits into from
Apr 8, 2025

Conversation

crop2000
Copy link
Contributor

@crop2000 crop2000 commented Dec 24, 2024

This pr grow out of the idea to have an enum to access parameters.
It also introduces a way to access the parameters via struct that mirrors the group structure.
In addition to this i also looked at the faust builder and introduce FaustArg enum to have a flexible basis to write builder functions.
I make use of the enum in faust-macro.
I also cleaned up my recent addition to the faust builder.
Changes to the builder should be backward compatible.
I use the builder in the exmple-f64.
I realized that the faust architecture file is not an ideal design pattern to interact with in a rust build script. In the example-f64 and faust-macro I show how one can create a architecture wrapper using quote.
i add a new jack example utilizing all the things i introduced.

@obsoleszenz
Copy link
Collaborator

obsoleszenz commented Mar 24, 2025

So i looked into this a bit now and i feel like this is a really nice overhaul of rust-faust.
Following things i would like to see:

  • Instead of introducing example-foo-new, just update the old example. Your improvements should define the state of the art of creating rust-faust projects, no need to keep the old examples around
  • I would like to not manually call std::process::Command in the examples and instead use FaustBuild

In general this feels like we could bump the crate version to 1.0.0 with your changes, really powerful stuff!

@crop2000
Copy link
Contributor Author

I kind of would like to do the integration of the new example and the update of FaustBuild in a separate PR.
What I could do is to add some comments to the changed examples and describe what it exemplifies.

@obsoleszenz
Copy link
Collaborator

I kind of would like to do the integration of the new example and the update of FaustBuild in a separate PR. What I could do is to add some comments to the changed examples and describe what it exemplifies.

Hmm then i think we need to reiterate on how to merge this pr. Merging it like this makes the example situation quite confusing to new adopters and i would like to prevent that. Can we split this pr into smaller chunks that are more easy to merge?

I'm thinking of following:

  • Extend FaustBuilder to use FaustArg
  • Extend FaustBuilder with a method like this: fn use_architecture(self, architecture_fn: Fn() -> TokenStream) -> Self
  • And then in another pr work on faust_json for the codegen?

@crop2000 crop2000 mentioned this pull request Mar 26, 2025
@crop2000 crop2000 force-pushed the macro_extensions branch 3 times, most recently from a416f9a to 547223f Compare March 26, 2025 19:20
@crop2000 crop2000 marked this pull request as draft March 26, 2025 19:31
@crop2000
Copy link
Contributor Author

this pr now only contains the examples but we agreed that the examples should be reduced to best practices

@crop2000 crop2000 force-pushed the macro_extensions branch 3 times, most recently from 79552d9 to 2bc46b7 Compare April 1, 2025 09:35
b.set_xml();
b.set_code_option(CodeOption::Double);
b.set_architecture(Architecture::Object(Box::new(ArchitectureUI {})));
b.build();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel file-64 is a bad example name. How about volume-f64 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the example is about using files and f64 ... it is not important that it contains a volume dsp. this is the naming convention used for all the examples

pub fn create(dsp_json: &FaustJson, dsp_name: &Ident) -> (TokenStream, bool, bool) {
let param_info = dsp_json.get_param_info();
create_from_paraminfo(&param_info, dsp_name)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the codegen should be a seperate crate, as i feel this is quite opinionated. Also I would prefer having the ui types/traits not bleed into faust-types. Also i propose renaming faust-json to faust-ui-description-json or maybe even merging faust-xml and faust-json under faust-ui-description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you worry about when the traits for ui are in the faust-types

type F;
fn get_value(&self, dsp: &D) -> Self::F;
fn get_enum(&self, dsp: &D) -> Self::E;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated above, I would prefer having the ui types/traits not bleed into faust-types but be in faust-ui-codegen or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you worry about when there are more types intoduced and included together? they don't need to be implemented.

self.compile_options.xml = true;
}

pub fn set_json(&mut self) {
Copy link
Collaborator

@obsoleszenz obsoleszenz Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn set_json(&mut self) {
pub fn write_ui_description_json(&mut self) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the current version i choose a different name

pub mod option_map;

pub trait ArchitectureInterface {
fn wrap(&self, builder: &FaustBuilder, dsp_code: TokenStream) -> TokenStream;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn wrap(&self, builder: &FaustBuilder, dsp_code: TokenStream) -> TokenStream;
fn architecture(&self, builder: &FaustBuilder, dsp_code: TokenStream) -> TokenStream;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the idea to call this function wrap is because this is what you do. you use the architecture object. and wrap it around the generated code.

pub mod macro_lib;
pub mod option_map;

pub trait ArchitectureInterface {
Copy link
Collaborator

@obsoleszenz obsoleszenz Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
pub trait ArchitectureInterface {
pub trait ArchitectureCodeGenerate {

Copy link
Contributor Author

@crop2000 crop2000 Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just Architecture? ok thats in use already. but i feel ArchitectureCodeGenerate is a bit long. ... i realize one problem the two example instances ArchitectureDefault ArchitectureUI could both be implemented as a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically the ArchitectureCodeGenerateObjectInterface 😅 doesn't even have a use case yet.

pub enum Architecture {
None,
Function(bool), //todo
Object(Box<dyn ArchitectureInterface>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Object(Box<dyn ArchitectureInterface>),
CodeGeneration(Box<dyn ArchitectureCodeGeneration>),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think when you look how the code is used in builder and the examples you can see how the naming convention is working.

#ui_reexport
}
}
}
Copy link
Collaborator

@obsoleszenz obsoleszenz Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also move this to the seperate ui codegen crate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you refer to the function that opens the json file and calls generate_ui_code i leave it in the builder because the builder takes care of files whereas faust-ui doesn't.


#[derive(Debug, Clone, Eq, EnumDiscriminants, EnumIs, EnumString)]
#[strum_discriminants(derive(Hash))]
pub enum CodeOption {
Copy link
Collaborator

@obsoleszenz obsoleszenz Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub enum CodeOption {
pub enum CodeGenerationOption {

To stay in line with the faust naming
image

A bit unfortunate that we will have FaustCodeGeneration and ArchitectureCodeGeneration, but well, i think we can explain the difference with good documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point for a longer name

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeOption is not a descriptive name. At least call it CodeGeneration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But those are not generating anything they are just options, flags or...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about CodeFlag?
I think this is clear enough in the context of the faust-builder

Copy link
Collaborator

@obsoleszenz obsoleszenz Apr 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But they are about the CodeGeneration, if we map something to a portion of the faust args, we should call it the same way and not introduce new/different/incomplete naming. And the faust cli clearly calls it Code generation options. Not flags. But we can rename those things later too, but those things should be settled when we cut a release so that we don't introduce breaking changes.

@crop2000 crop2000 force-pushed the macro_extensions branch 3 times, most recently from e28f3b0 to 8e9de13 Compare April 3, 2025 13:04
fn main() {
println!("cargo:rerun-if-changed=dsp");
let mut b = FaustBuilder::default_for_file("dsp/volume.dsp", "src/dsp.rs");
b.set_architecture(Architecture::default());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the default architecture set by default_for_file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently the default_for_file sets as little as possible so it does set Architecture to None which currently applies default

default(builder, &ts)
... not sure if this is the best idea.

pub enum Architecture {
None,
Function(&'static dyn Fn(&FaustBuilder, &TokenStream) -> TokenStream),
Object(Box<dyn ObjectInterface>),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of Object over Function?

Copy link
Contributor Author

@crop2000 crop2000 Apr 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An object could carry other information that is needed when apply() is called. For example one could prepare another string or file name that is used for generating the wrapping content. An example for that would we module name. If it would not be provided by the builder.

}
}
}
pub static DSP_UI: DspUiVolume = DspUiVolume::static_ui();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the differentiation between Active & Passive?
Is Active Read + Write and Passive only Read?

Copy link
Contributor Author

@crop2000 crop2000 Apr 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Active are sliders and one should set the value. i think one should not design ones programs to also read them. Passive are meters and one should read the value.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Active are sliders and one should set the value. i think one should not design ones programs to also read them. Passive are meters and one should read the value.

Why should one not read them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i mean one should not depend on reading the slider values from the dsp struct because one anyway need to send them there from another context.

@crop2000 crop2000 marked this pull request as ready for review April 5, 2025 22:35
refactor faust-macro: refactor to make use of ui generated from json
make use of architecture functions by default
refactor faust-build
rewrite examples
make more use of clippy lints

remove default private module around dsp code and create a public module if apropriate
 ... some code cleanup is possible because of this
@obsoleszenz obsoleszenz merged commit c720d04 into Frando:main Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants