Skip to content
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

Add step, step_select, mix to spirv_std::MathExt #223

Closed
wants to merge 2 commits into from
Closed

Add step, step_select, mix to spirv_std::MathExt #223

wants to merge 2 commits into from

Conversation

fu5ha
Copy link
Member

@fu5ha fu5ha commented Nov 10, 2020

Does what it says on the tin once again... step is of the form x.step(edge) rather than edge.step(x), which I think makes more sense even though the arguments are 'reverse' order compared to glsl.

@fu5ha fu5ha added t: enhancement A new feature or improvement to an existing one. t: ergonomics Language ergonomics discussions and experimentation c: spirv-std Issues specific to the spirv-std crate labels Nov 10, 2020
@fu5ha fu5ha requested a review from repi November 10, 2020 22:17
Copy link
Contributor

@khyperia khyperia left a comment

Choose a reason for hiding this comment

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

neat! we could bikeshed all day on API design, but from a technical point of view, looks great. (asked our resident shader expert on her opinions, too~)

@@ -82,4 +100,24 @@ impl MathExt for f32 {
fn copysign(self, sign: f32) -> f32 {
unsafe { core::intrinsics::copysignf32(self, sign) }
}

fn step(self, edge: f32) -> f32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should probably be an intrinsic that compiles to the spir-v step instruction, but whatever, who cares right now. I agree with the x.step(edge) order (and Freya does too ✨).

Copy link
Member Author

Choose a reason for hiding this comment

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

afaik step is almost certainly compiled down to an if like this anyway so ✨

}
}

fn mix(self, other: f32, a: f32) -> f32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer this to be lerp, but that's mostly bikeshedding and I don't really care. Also, I think I'd prefer t.lerp(a, b) instead of a.lerp(b, t), although Freya's a lil sketchy here and thinks that both ways are valid interpretations, and almost wouldn't want an "object"-style method to avoid that ambiguity, haha. (t.lerp(a, b) makes sense in shaders, a.lerp(b, t) makes sense when animating objects)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I originally agreed with that, but one thing that one of your comments brought up actually is that I feel like lerp has a bit more of a stringent mathematical definition/interpretation that doesn't really allow something like fn lerp(self: Vec3, other: Vec3, a: Vec3) whereas with a more loosey-goosey definition of mix it can make sense (and is sometimes useful).

Copy link

@FreyaHolmer FreyaHolmer Nov 10, 2020

Choose a reason for hiding this comment

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

I'm a little torn on this one, but I think if we consider inverse lerp, I think it's crystal clear that the value parameter should go first, because in 99% of cases, you are remapping a value into a 0-1 range

t = InvLerp( a, b, value )
t = value.InvLerp( a, b )

and looking back at lerp, we're usually blending between two states, and having those states next to each other to signify the "input range" makes a lot of sense

v = Lerp( a, b, t )
v = t.Lerp( a, b )

And with the "remapping to a range" perspective, this one too is remapping a t value into another range. The one thing that makes me uncomfortable with this one though is that t is usually a float, but it can return a vector (or vice versa) which feels a little spooky to me for some reason~ But in the context of value remapping it kinda makes sense. Just like you could have a remap function which would read pretty naturally too:

vOut = vIn.Remap( iMin, iMax, oMin, oMax )

Copy link
Contributor

Choose a reason for hiding this comment

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

or be really spicy with t.mix(a..b) >:D (/s. mostly.)

}

fn mix(self, other: f32, a: f32) -> f32 {
self - (self + other) * a
Copy link
Contributor

Choose a reason for hiding this comment

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

also wait, is this correct? wolframalpha says this is equivalent to self * (1 - a) - other * a, which, I think lerp/mix is self * (1 - a) + other * a? I think this should be self + (other - self) * a? it is midnight and I am doubting myself though D:

Choose a reason for hiding this comment

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

pretty sure this one is incorrect yeah! usually I see either of these two:
v = (1-t)a+bt
v = a+(b-a)t

I think I recall that the first one is more numerically stable as t approaches 1? in case that matters here, not sure if it does

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes you are correct, i didn't distribute the negative x)

Copy link
Contributor

@repi repi left a comment

Choose a reason for hiding this comment

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

My general thinking with MathExt in here and in glam is to only expose functionality that exists in Rust std::f32: https://doc.rust-lang.org/stable/std/primitive.f32.html. Or at least only functionality that we have in both spriv_std here and in glam so it for our shader is the same with the f32 type as the Vec2/Vec3/Vec4 types (overall).

That would include trunc and fract in this PR, but not the shadery step, step_select or mix.

The goal with that would be three fold:

  1. Ultimately would want to use Rust std inside our shaders with the exact f32 functionality it exposes and without doing use spirv_std::MathExt or even bringing in spirv_std at all (for library crates that don't need the attributes other functionality).
  2. (very related to 1) Keep it as small as possible as wrappers over intrinsics and libm functionality, not opinionated additional functionality. That can and will be layered on top instead.
  3. Avoid bike-shedding about what would be a good shader math library with additional types and functionality specifically for making it is easy to build shaders.

What do you all think about those as goals / approach?

Doing work on #3 here with coming up with ergonomic shader-style functions is good also, but I do think that should happen outside of this trait and mostly start in the actual shaders using it first before being "promoted" to shared "shader std" functionality.

That makes it a lot faster to iterate on as well and one can discuss more clearly the tradeoffs of specific functions and where the functionality should be as there are many options:

  • Could be (and start in) just our specific shaders and their utility crates
  • Could be in our Embark-opionated higher-level multi-target math library on top of glam ("macaw", not published)
  • Directly in glam itself
  • Here in spirv-std / "shader std"
  • Or ultimately in Rust std for f32 primitive types and everyone (can be hard sell).

I do think at a minimum it is important to distinguish between these levels of math functionality and destination of it as well. And it does have technical connections also in that some is just direct wrappers for Rust core intrinsics, some (most of MathExt) is replacement for Rust std f32, some will be through libm replacement

@fu5ha
Copy link
Member Author

fu5ha commented Nov 10, 2020

It just feels awkward to me to have like 4 different places where you're always juggling between adding functionality... i.e. I'm thinking to myself "Okay I have this piece of functionality that I know a large number of shaders are going to want to use (step, step_select, mix in this case). Currently, they're just functions in a utils module in the shader crate. Now how should I promote this so it's more integrated and don't have to rewrite it? Do we want this in glam? Well, not really, since glam is basically housing vector math functions rather than helper functions on primitive types... hm, we could perhaps put it in macaw but that feels weird since this is almost 'lower' level than glam... okay well that sounds like we might want this in spirv_std I guess, since that's where other 'primitive' math functions are... but I guess we only want the true primitives from regular Rust std?"

@repi
Copy link
Contributor

repi commented Nov 10, 2020

It is indeed more work to intentionally design and reason around it, but also just adding stuff into spirv-std is not a general easy solution either as that doesn't implement the functionality on glam or any other higher-level math libraries. it just does it for f32.

Which is for example while we converted our first shaders in this repo to glam we realized that we needed exp and pow in glam as we are doing vector operations on it. That we can't solve in spirv-std, there we can only add it for f32, but we needed it for the vector types also which will be added (bitshifter/glam-rs#91).

It gets tricky with that f32 & vector type split. Think the only solution if we want to have say a mix or step function both on f32 and our vector types is to have it on a crate that is above glam, which is our macaw, that could have this MathExt´ and implement it both for f32` and the glam vector types that it re-exports.

Definitely requires a bit thinking and designing how to approach it. And I don't mean we need all 4 levels and different functions everywhere, but do need an approach and pick 1-2 levels and something that has a decent workflow to define this consistently for f32 and vector types.

@fu5ha
Copy link
Member Author

fu5ha commented Nov 11, 2020

Alright! Oh, we should remove saturate from spirv_std then.

@repi
Copy link
Contributor

repi commented Nov 11, 2020

Think this could be a good topic to discuss the approaches and tradeoffs in the next meeting (#227), pretty sure we can find and pick a path and unblock ourselves but also have a direction towards how it all will fit together

@repi repi mentioned this pull request Nov 11, 2020
@fu5ha fu5ha mentioned this pull request Nov 11, 2020
@fu5ha fu5ha closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: spirv-std Issues specific to the spirv-std crate t: enhancement A new feature or improvement to an existing one. t: ergonomics Language ergonomics discussions and experimentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants