Replace make_float4 with Float4::expr, etc.

[entropylost]

Summary of Changes:

This pull request changes all use of make_xxxxN(...) with XxxxN::expr(...).

Why?

The primary reason for this is that the make_float4 type functions are rather deceptive - the direct reading would imply that it'd make a Float4, but it actually makes a Float4Expr. Additionally, it was rather inconvenient to discover how to create Float4Exprs out of normal values, as the Float4Expr::new didn't support taking Into<F32> types. Replacing these functions with Float4::expr functions fixes this issue, as well as decreases number of imports necessary (if not using a glob luisa::* import) and namespace pollution, and also cleans up the code, as the make_xxxxN functions had to be special-coded.

Concerns:

• There is still a Float4Expr::new( function remaining. I kept this as a way of building Float4Exprs without having a possibility for side effects due to the Into type might be useful occasionally, but removing it might be helpful.
• This is a breaking change, and will require refactoring of the renderer dependent on this.

[entropylost]

I'd like to know if there's a way people want me to submit pull requests, and also what types of pull requests people are interested in accepting (eg. breaking changes)? I'm going to be using this for my application, so I'll probably have more things along these lines.

[shiinamiyuki]

Thank you for your interest in our project. Yes I agree with your point. The current design was to keep the DSL similar to its C++ counterpart. However, as your pointed out, it may not be optimal for Rust.
Do you think it would be also good idea to replace XXXExpr::new to XXX::expr for custom types?

[shiinamiyuki]

Feel free to open whatever pull request you want, even if it includes breaking changes. We welcome new ideas!

[entropylost]

Thank you for your interest in our project. Yes I agree with your point. The current design was to keep the DSL similar to its C++ counterpart. However, as your pointed out, it may not be optimal for Rust. Do you think it would be also good idea to replace XXXExpr::new to XXX::expr for custom types?

@shiinamiyuki Hmm, I'm unsure. Part of the issue is that people wouldn't necessarily want to create a large struct with a really large series of news, and with the automatic generation, it'd make changing ordering in the struct a breaking change. Perhaps it might like make sense to add like a XXXExprData type, and then you can do

XXXExpr::from_components(XXXExprData {
    a: 10,
    b: Float2::expr(1.0, 5.0),
});

Although this is rather verbose. Edit: Because of the Expr<__> syntax, I'd consider making the Float2Expr, etc notations all private, as that'd make it easier to keep track of imports. Although then for consistency we'd want to make F32 private; would that provide issues?

It also might make sense to replace impl Into<Expr<T>> with a special type impl IntoExpr<T>, which specifically takes either T or Expr or Var, and use that everywhere, as I feel rather... iffy about using the normal Into since it can be extended randomly and generally is hard to keep track of (not to mind rust-analyzer gets confused).

Anyways, I think I'll try a couple of different ways of doing this.

[entropylost]

Also, would anybody be opposed to splitting up the lang/mod.rs file into different places for PrimExpr, etc? It's somewhat inconvenient for me to dig in there sometimes, but some people seem to like large files?

[shiinamiyuki]

Although this is rather verbose. Edit: Because of the Expr<__> syntax, I'd consider making the Float2Expr, etc notations all private, as that'd make it easier to keep track of imports. Although then for consistency we'd want to make F32 private; would that provide issues?

Correct me if I'm wrong, I don't think it is possible to make XXXExpr private since they are ultimately exposed through Value::Expr, forcing it to have the same visibility as the value type.

XXXExpr::from_components(XXXExprData {
    a: 10,
    b: Float2::expr(1.0, 5.0),
});

There is a struct_ macro that does, albeit it uses struct_!(XXX { ... }) syntax rather than struct_!(XXXExpr { ... }). It also breaks rust-analyzer but this might be fixable. I admit the library is less polished when it comes to the consistency of whether using XXX or XXXExpr.

Also, would anybody be opposed to splitting up the lang/mod.rs file into different places for PrimExpr, etc? It's somewhat inconvenient for me to dig in there sometimes, but some people seem to like large files?

Feel free to do so.

[entropylost]

@shiinamiyuki

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8c9ede544802d6f39e1827cf87c5318 this seems to work

mod bar {
    mod foo {
        pub struct A;
    }
    pub type B = foo::A;
    pub fn get_b() -> B {
        foo::A
    }
}

fn main() {
    let x = bar::get_b();
}

compiles, but adding let y = bar::foo::A breaks it.

And hmm, I'll check the struct_! thing.

Also rust-analyzer generally just doesn't work within macros for displaying the type inference things at all; that's pretty much unfixable on this end.

By the way, is there any place where you use (or intend to use) a VarProxy that isn't just { node: NodeRef, _marker: PhantomData }? Because then we could just replace all of them with a struct Var { node: NodeRef ...} (although there might be some fiddling necessary - you'd have to generate a trait XXXAccessors for the .x() style methods).

[entropylost]

Alternatively, if we're willing to make a mess of things and use nightly, it'd be possible to use arbitrary_self_types with Deref<Target = T> for Expr<T>, and directly allow doing impl XXX { fn x(self: Expr<XXX>) -> Expr<u32> { ... }

[shiinamiyuki]

mod bar {
    mod foo {
        pub struct A;
    }
    pub type B = foo::A;
    pub fn get_b() -> B {
        foo::A
    }
}

fn main() {
    let x = bar::get_b();
}

compiles, but adding let y = bar::foo::A breaks it.

Thank you! This looks great! One question remains: how to handle method defined on XXXExpr

#[derive(Value, Clone, Copy)]
#[repr(C)]
pub struct Onb {
    tangent: Float3,
    binormal: Float3,
    normal: Float3,
}

impl OnbExpr {
    fn to_world(&self, v: Expr<Float3>) -> Expr<Float3> {
        self.tangent() * v.x() + self.binormal() * v.y() + self.normal() * v.z()
    }
}

impl Expr<Onb> direct is an error. I can see one way is to define a new trait:

trait ToWorld {
    fn to_world(&self, v: Expr<Float3>) -> Expr<Float3>;
}
impl ToWorld for Expr<Onb> {
    fn to_world(&self, v: Expr<Float3>) -> Expr<Float3> {
        self.tangent() * v.x() + self.binormal() * v.y() + self.normal() * v.z()
    }
}

while I can use proc-macro to make it less verbose. It requires a new trait to be generated for each impl block.

Also rust-analyzer generally just doesn't work within macros for displaying the type inference things at all; that's pretty much unfixable on this end.

It works to some extent. Sorry, I should make a correction that I originally meant it breaks rustfmt.

By the way, is there any place where you use (or intend to use) a VarProxy that isn't just { node: NodeRef, _marker: PhantomData }? Because then we could just replace all of them with a struct Var { node: NodeRef ...} (although there might be some fiddling necessary - you'd have to generate a trait XXXAccessors for the .x() style methods).

Maybe? The main reason behind current design is to enable impl TraitInAnotherCrate for XXXVar and similarly for XXXExpr.

[entropylost]

Maybe? The main reason behind current design is to enable impl TraitInAnotherCrate for XXXVar and similarly for XXXExpr.

Ah yeah, being able to add impls would be kind of important. Although I don't think that adding external trait impls might be the most important. Whatever, the current version seems to work well enough. Also, for the impl Expr<Onb> thing, you could just put the impl blocks in the impl priv::Expr<Onb>, or even have an alias type OnbExpr = priv::Expr> and then impl OnbExpr.

[entropylost]

@shiinamiyuki closing because it's incorporated into #10.